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

Define requirements in composer.json #3754

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Feb 5, 2023

Following some feedback on #3752 about an extension that I'd forgotten to add to composer.json, I have set up maglnet/composer-require-checker to automatically detect such errors in future. While doing so, I noticed that the following extensions are already required, but were not listed in composer.json:

Additionally, there was no autoload section in composer.json, which I have now added.

I had to create configuration for the checker tool to avoid it complaining about a large number of (global) constants that are defined here.

@jrfnl
Copy link
Contributor

jrfnl commented Feb 5, 2023

@fredden Thanks for the PR. I can't say anything about whether it will be accepted or not, but can give you some feedback in general terms, but in the end, it's @gsherwood's decision.

  • Most of the extensions you now listed in require are only needed for specific features, not for the application in general.
    As a matter of course, extensions which ship with PHP and can not be disabled do not need to be listed.
    The five extensions you've now added all ship with PHP and are all enabled by default (though most can be disabled).
    In most cases, the uses of those extensions will be conditional on the extension being available, which means the extension is not actually required (though could be added to suggest with a hint as to which functionality requires the extension).
    It could be that in one or two cases, the "conditional use" has fallen through the cracks, in which case, that should be fixed (instead).
  • The autoload sections are redundant as PHPCS handles this in the application bootstrap.
    Additionally, a PSR4 autoload section for the src directory can actually break the PHPCS translation between classes and sniff names, which is needed for the correct handling of (custom) rulesets.
    Furthermore, PHPCS also supports PHIVE, PEAR and git clone installs, so relying on Composer for the autoloading would be wrong and unstable.
  • In my experience, the general tendency for PHPCS has been to have a minimal set of dependencies, even dev dependencies, so not sure adding a new tool will be welcome.

Sorry to be a party-pooper.

@fredden
Copy link
Contributor Author

fredden commented May 16, 2023

It could be that in one or two cases, the "conditional use" has fallen through the cracks, in which case, that should be fixed (instead).

Would it be helpful for me to open pull requests for each of the identified extensions, and add a "conditional use" check around these? We could then move these extensions from the require section to suggest.

@fredden fredden marked this pull request as draft June 7, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants