Removed useless variables for return statements

[sreichel]

Description (*)

Remove unused variables.

Contribution checklist (*)

• [x] Pull request has a meaningful description of its purpose
• [x] All commits are accompanied by meaningful commit messages
• [x] All automated tests passed successfully (all builds are green)

[tmotyl]

Hi @sreichel what is the reasoning for this change? It's easier for debugging when the variable is there (e.g. stepping with xdebug). Does it bring any value?

[sreichel]

@tmotyl just some cleanup. It's used ony in a very few places, everywhere else is no varaiable set.

[fballiano]

I don't know, it's a very big commit on a subjecting matter. Yes, I like the "new" code more, at the same time I wouldn't have modified 170 files just for a bit cleaner code.

Anyway this PR doesn't do any harm so I think it's ok to merge it.

@Flyingmana what do you think?

[fballiano]

@sreichel could you resolve the conflicts so we can merge this PR?

@luigifab would it be ok with you if we merge this PR and you open a new one to fix your notes (which are correct)?

[tmotyl]

Please dont do this. This makes debugging harder and doesnt solve any issue (this cs rule doesnt make code easier to read in most cases). Lets focus on having phpstan related patches merged, or introducing cs rules which brings consisteny and makes code easier to read.

Changes like that should be introduced together with php-cs-fixer rule to make sure new code is compliant.

[fballiano]

IMHO @tmotyl and @sreichel should decide what to do on this one, also there are some conflicts to be fixed now

[luigifab]

@fballiano yes I can make a second PR.

[luigifab]

One of the inspection of PHPStorm agree with these changes 😸.

[fballiano]

One of the inspection of PHPStorm agree with these changes 😸.

how do you use that? curious to learn

[tmotyl]

@luigifab which rule? I see much bigger benefit in proceeding with changes like: https://github.com/OpenMage/magento-lts/pull/2055 (I have few more changes waiting for this PR to be merged) before changes like removing variables in return statements. The change I linked is improving quality of work of developers a lot (less confusion on return types, better suggestions in IDE, less type related bugs.

[luigifab]

Yes, this is probably better an automated tool, but I don't understand how it works.

This is: Unnecessary local variable (example with Nexcessnet/Turpentine module):

[tmotyl]

ah sorry @luigifab you wrote phpStorm, and I read phpstan;) yea this rule is quite annoying in phpstorm, fortunately is easy to disable in the phpstorm config

[fballiano]

This PR have a lot of comments to implement and some conflicts that I'm not sure about, can somebody please take a look at this?

[sreichel]

@sreichel could you resolve the conflicts so we can merge this PR?

As soon my phpStorm licenese is renewed. Hope next week ...

[sreichel]

There are a lot. Would fix them in a separate PR.

[sreichel]

I see much bigger benefit in proceeding with changes like: https://github.com/OpenMage/magento-lts/pull/2055 (I have few more changes waiting for this PR to be merged) before changes like removing variables in return statements. The change I linked is improving quality of work of developers a lot (less confusion on return types, better suggestions in IDE, less type related bugs.

@tmotyl i highly appreciate your work and its defenitly more important then removing useless returned variabless, but ... this PHPStan sh** would not run, if i had not spent a few hours/days/weeks to add thousends of DOCblocks ...

It's a cosmetic change, but why not have better/cleaner code?

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit 620cc0a4. ± Comparison against base commit 3dda4baa.