Add exclude directive for maybe-ASP-open rule

[fredden]

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

[jrfnl]

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]

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:

• https://github.com/magento/devdocs/blob/2.0.8/_includes/install/php-config.html#L20-L24
  • http://web.archive.org/web/20160304134209/http://devdocs.magento.com/guides/v2.0/install-gde/prereq/php-ubuntu.html#instgde-prereq-timezone
• https://github.com/magento/devdocs/blob/2.1.18/guides/v2.1/install-gde/prereq/php-settings.md?plain=1#L32-L36
  • http://web.archive.org/web/20170729020318/http://devdocs.magento.com/guides/v2.1/install-gde/prereq/php-settings.html

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.

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).

[jrfnl]

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]

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