Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exclude directive for maybe-ASP-open rule #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Jun 16, 2023

This directive is currently being excluded individually in the Magento2 ruleset. Moving it here will allow us to use this ruleset instead of maintaining the main PHP Compatibility ruleset going forward.

https://github.com/magento/magento-coding-standard/blob/0f81833b28d6fc3d799986ebcfa7fee659927ff4/Magento2/ruleset.xml#L765-L766

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this exclusion. This is not a false positive. This is Magento using an placeholder delimiter which will break on PHP < 7.0 with the asp_tags ini directive set to 1.

This is not something which Magento can polyfill or change at run-time either as the asp_tags directive cannot be changed at run-time.

And while the latest versions of Magento2 do not support PHP < 7.0 anymore, Magento 2.0 and 2.1 still did.

For this to be added/accepted, it would have to be made clear in the README that the ruleset is only applicable for a certain Magento x version and up, which I'm not sure is wise in the context of Magento extensions ?

@fredden
Copy link
Contributor Author

fredden commented Jun 17, 2023

Yes, that's a fair response. I agree that this isn't a false positive. I do however think it should be excluded from the rule-set here, and without a corresponding note in the README.

Turning off asp_tags was listed as a system requirement for Magento 2.0 and 2.1:

and recommended on this support page: https://support.magento.com/hc/en-us/articles/360034599631-PHP-settings-errors

This particular rule is already specifically disabled in certain files, for example: https://github.com/magento/magento2/pull/31913/files. It's also already disabled in the official Magento2 coding standard (see https://github.com/magento/magento-coding-standard/blob/0f81833b28d6fc3d799986ebcfa7fee659927ff4/Magento2/ruleset.xml#L766) and has been since that standard started using PHPCompatibility (see magento/magento-coding-standard@39de3e6).


As part of our automated pipeline, we run PHPCompatibility across our Magento modules and projects. Currently I'm using the PHPCompatibility ruleset with one exclusion (PHPCompatibility.Miscellaneous.RemovedAlternativePHPTags because I can't target only PHPCompatibility.Miscellaneous.RemovedAlternativePHPTags.MaybeASPOpenTagFound from the command line). I'd like to instead use this rule-set and not have any exclusions at our end.

Screenshot_2023-06-17_13-05-34

I think it makes sense for the Magento2 coding standard to use this rule-set instead of pulling in all of PHPCompatibility and making the exclusion locally (links above).

Screenshot_2023-06-17_13-07-40

@jrfnl
Copy link
Member

jrfnl commented Jun 17, 2023

Turning off asp_tags was listed as a system requirement for Magento 2.0 and 2.1:

Listing something as a requirement and actively enforcing it (by, for instance, checking the ini value and exiting out if the value does not comply with the requirements) are two different things.

Keep in mind, this ruleset is not just for Magento Core, but like the other CMS rulesets, also intended for extension/plugin/theme authors in the context of Magento and even for end-users of Magento (who may want to check if the setup they have, including all extras, can be safely moved to a server running a higher PHP version).

While Magento Core does not want to see the notice as they've covered the issue with a recommendation in the docs (and I do understand that), it may still be important for extension/plugin authors and end-users of Magento to see the message and verify that their settings are correct.

@jrfnl
Copy link
Member

jrfnl commented Jun 17, 2023

Oh and I'm also thinking: should this ruleset by renamed PHPCompatibilityMagento2 ? And should there be a separate ruleset for Magento 1 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants