Add opt-in for sorted use statements

The proposed solution in the MR allows to set a variable _PHPCS_OPT_IN_CONFIG=sort-sniff/ so that the alternative config would be loaded.

This way could be a blue print for other alternatives in the future as well.

we could offer an opt-in for projects that still want to use that sniff.

I would suggest just encourage the use of the phpcs config file.

• It allows users writing code locally to use the same config as the CI runs.
• It is one less option that needs to be tested when templates change
• It is one less config option and file that needs to be maintained.

If gitlab_templates is going to add this it should also probaly add the case insensitive option too (as it is what has been enforced for years even though it was not an approved standard).

What is gitlab_templates plans for this option if core settles on a standard that does not match the committed option (in other words how involved does templates want to be in encouraging deviating from the “standard”) ?

What precedent does this set for gitlab_templates maintaining multiple options in the future? What about the next time a similar conflict occurs in coder? Is this project going to maintain to an exponential number of templates?

Is gitlab_templates prepared to start having to code custom rules if for some reason a conflict arises?

Leaving this as NR as those questions are out of my scope to answer and are intended to just get gitlab_templates to think about the impact of options rather than rushing head first as it often does while also pushing for thinking about local development and maintainer responsibilities at the same time.

I see your points @cmlara and sure enough, there are valid and important arguments.

On the other hand, providing your own phpcs.xml.dist file as part of the code base comes with at least 2 downsides:

• Adding such a file to every project is cumbersome, for some maintainers 50, 70, or even more
• As soon as modules bring their own file, they're cut off from maintenance of the upstream default. They will not benefit from any bug fixes or improvements from the default config of the templates any longer

So, yes, adding both options, case-sensitive and case-insensitive sorting would actually be great. But I leave the MR as is for now, until we got more opinions and a conclusion eventually.

I agree with @cmlara that we don't need to cater for variations of coding standards that can already be fully customised by the maintainer via a phpcs.xml.dist file. Just to respond to the points from the above -

Adding such a file to every project is cumbersome,

The latest change to Coder removes the sniff, so maintainers are not forced to add their own file. If they didn't care about the use statement order before the sniff was added, then they won't mind if the sniff is removed. And if they did care about the order before it was added to Coder then they would already have a custom config file which added it directly. I don't see why a maintainer with 50 projects would suddenly have the need to add the file to every one of the projects, just for this latest Coder change.

As soon as modules bring their own file, they're cut off from maintenance of the upstream default. They will not benefit from any bug fixes

That is true, but the Gitlab Templates asset file is minimal, and we don't anticipate making too many changes. Of course, we can never tell what might be needed in the future, but it is not a file that is expected to change.

What might be more useful and generic would be a way for a maintainer to specify some additions to the default phpcs.xml.dist file via variables, so they can choose to add enhancements but still use the default gitlab templates asset. That way, if there were changes to the default they would still get them. This might not be a great idea, as it may cause us maintenance/bc problems later, but it's worth thinking about. We can document how to specify the additions, but gitlab templates would not actually provide them, just provide the generic process to implement them.

Adding such a file to every project is cumbersome

I would suggest they are about equally as cumbersome.

cp /tmp/phpcs.xml.dist ./; git add phpcs.xml.dist; git commit -m ‘use ordered sniff’;
Vs
yq ‘.Variables += {NewVariable: NewValue}’ .gitlab-ci.yml; git add .gitlab-ci.yml; git commit -m ‘use ordered sniff’;

Note: the above are rough pseudo code intended to illustrate similar levels of work and that one has the complexity of editing lines in existing files vs adding a new file. The only edge case where this dramatically changes is where a maintainer has a single project they use as a base template to inherit all their other projects(I do this on some projects) , in which case it can be done with a single point change vs a multi point change.

I still belive the custom phpcs to be the better method for about equivalent effort and burden to a project long term, while making it easier for everyone who contributes (if I pickup a random project how does my local install of phpcs know which standard the maintainer prefers, it needs to be in the phpcs.xml.dist).

I agree with the comments written above. I don't think we should be catering for these small changes in behaviour to PHPCS in the templates, especially when deviating from the standard is a maintainer's choice and we already give a mechanism to override what we provide (eg: adding the own module's phpcs file). It would have been great that phpcs would accept other flags so we could have leveraged $_PHPCS_EXTRA, but I don't think there was a flag available for this case.

I understand that it might be confusing or frustrating in this case, as the standard was added, then removed, but from our point of view, I think that we don't need to change anything.

--

@jurgenhaas - what I've suggested to other maintainers with a considerable amount of modules under their responsibility is to have somehow a central repository of assets, which they can then reference within their templates.

So, for example, you could have https://gitlab.lakedrops.com/gitlab-ci-cd/drupal/-/tree/main/assets/phpc..., that you would control centrally, and then have all modules copy this file in the before_script step. You'd obviously need to do this change once in each module at first (no matter which solution this would need to happen, whether it's a variable, or anything else), but after that initial change, you'd control all the PHPCS standards from all your modules in one place.

I'm happy to continue the discussion if needed tho.

jurgenhaas → closed merge request !298