Remove forbidden @author tag from Magento_Catalog (part 3)#36989
Remove forbidden @author tag from Magento_Catalog (part 3)#36989fredden wants to merge 9 commits intomagento:2.4-developfrom
@author tag from Magento_Catalog (part 3)#36989Conversation
|
Hi @fredden. Thank you for your contribution! Add the comment under your pull request to deploy test or vanilla Magento instance:
❗ Automated tests can be triggered manually with an appropriate comment:
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
|
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
1 similar comment
|
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
leonhelmus
left a comment
There was a problem hiding this comment.
I have added some comments to make sure that the changes were documented, because it could influence the behavior of the method.
| $finish = $this->getLastPageNum(); | ||
| $start = $finish - $this->_displayPages + 1; | ||
| } else { | ||
| $start = 1; |
There was a problem hiding this comment.
@fredden This seems to be a change that was not written in the pr. It does seem logical that it was added, but it could influence the method.
There was a problem hiding this comment.
Without this change, the linter complains that $start and $finish are potentially undefined when calling range($start, $finish); on the next line of code. This seems like a necessary change to ensure the code functions as expected. While technically out of scope for "remove a comment" (this pull request), this pull request also contains a lot of other out-of-scope changes (like updating the license holder(!) and defining class constant visibility). How should we proceed?
| } | ||
| } | ||
| if (isset($options[$value])) { | ||
| return $option[$value]; |
There was a problem hiding this comment.
Although this seems logical i would add it in the pr, because this could be a change in behavior for this method.
There was a problem hiding this comment.
This is a classic typographical error which was highlighted by the linter. Due to the way that Adobe have configured their static code analysis, problems like this go unnoticed for a long time. Would you prefer a separate pull request for this particular change?
|
Adding the same priority as #36976 (comment) |
|
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
|
@magento run all tests |
|
@magento run all tests |
|
Hello @fredden, Thanks for the contributions! Could you please take a look at this #36989 (review)? Thanks! |
|
Hello @fredden, This is a reminder: Could you please take a look at this #36989 (review)? Thanks. |
|
@engcom-Dash I have responded to Leon's comments. |
|
Hello @leonhelmus, It looks like @fredden has responded to your review comments. Could you please take a look and provide your feedback? Thanks! |
|
@magento run all tests |
|
@magento run all tests |
Description
According to https://devdocs.magento.com/guides/v2.4/coding-standards/docblock-standard-general.html#documentation-space, the
@authortag is not permitted in Magento. This pull request removes this tag from (some of) theMagento_Catalogmodule. Given there are so many instances of this tag, I've opened a small pull request to get the process started. I expect that the linter will force me to fix several other coding standards violations on the way, so having a smaller pull request means that task is easier to manage. I plan to open more pull requests to tackle the other instances of this tag.See also magento/magento-coding-standard#382 and magento/magento-coding-standard#167
See also #36304, #36976, #36977, #36978, #36979, #36980, #36981, #36987, #36988
Manual testing scenarios
There are not code changes in this pull request. This pull request only removes forbidden comments.
Contribution checklist