- Issue created by @the_g_bomb
- Merge request !12Issue #3495504: Fix Route "admin/config/people/accounts" does not exist. → (Open) created by the_g_bomb
- First commit to issue fork.
- 🇬🇧United Kingdom the_g_bomb
We should keep the solutions specific to the issue scope. PHPCS issues should be fixed in an issue of its own.
drush site-install is an alias of site:install (https://www.drush.org/13.x/commands/site_install/) so either would work, but you have highlighted an issue in the instructions. drush cannot be called where you changed it as drush has not yet been installed.
I'll further change the instructions.
- 🇬🇧United Kingdom oily Greater London
"PHPCS issues should be fixed in an issue of its own." The gitlab pipelines run PHPCS and PHPSTAN tests on every issue commit. Fixing PHPCS and PHPSTAN errors is all part of the normal workflow. The PHPCS and PHPSTAN checks run first before the tests are run. So to get the code to even reach the PHPUNIT tests in the pipeline you have to first fix any PHPCS and PHPSTAN failures.
In this case the pipeline was failing due to several minor PHPCS errors/ warnings. After fixing those the pipeline progressed to the PHPUNIT tests. It passed them. That means the code is quality assured- though it is still possible for 'bad' code to get through hence the need for review. I only mentioned that I had done a PHPCS fix because ideally someone who is 'neutral'/ unbiased does the code review. That is someone who has not committed any cod.
- 🇬🇧United Kingdom oily Greater London
As far as the 'site-install' command goes, I simply copied the DDEV commands one at a time (I did change the order slightly but not in a way that makes any difference). It is rare that anyone lists out a repeatable set of commands like you do. It makes it easy to reproduce the issue. If 'site:install' is an alias of 'site-install' I used 'site-install' and it was not recognised for me. If it works for you using your particular versions of ddev and drush fine by me. You have recorded the discrepancy so others will no doubt try both variants if they have to.
- 🇬🇧United Kingdom the_g_bomb
Thanks again.
My approach for contrib modules is that patches often have to be applied to in-use modules. If several patches change things like version requirements or PHPCS errors, then it can prove tricky to get the correct cocktail of patches to apply in composer.
For example, an unneeded version change has been applied to 📌 username_enumeration_prevention compatiblity Needs work , which means this patch and that one cannot both be applied to the module without extra work.
If we keep the scope tight we might limit the overlap, especially if the patches aren't being merged in a timely manner. Lets hope this one goes through quickly.
- 🇬🇧United Kingdom oily Greater London
Hi, Yes I agree. Keeping atomicity of issues is best. I am not sure how gitlab is setup for this module. I think would be best if merges are only allowed by a maintainer if PHPCS PHPSTAN etc are green.
I may apply as a maintainer. That way I can see if there are rules setup for merges and if I can develop them so more robust. Also I was thinking could try creating test coverage for this one issue as a starting point. Or to create a few basic tests. Possibly best to create an issue for exploring such matters.. I am on Slack in #contribute channel and #needs-review-queue-initiative. Hoping to get more involved in Drupal.org and would be interested to hear your ideas.