Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Once coder-project has released the same version 8.3.1 version at packagist.org
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
drupal/coder has been updated to ˆ8.3.1. You may need to re-install coder if you have automated coding standards checks as part of your workflow, since the update includes new coding standards checks.
For example:
# rm -rf vendor/drupal/coder vendor/squizlabs
# composer install
Comment | File | Size | Author |
---|---|---|---|
#31 | 3002206-2-31.patch | 24.88 KB | alexpott |
Comments
Comment #2
truls1502Then, here is the patch for only core-part which need to upgrade on https://github.com/drupal/core and https://packagist.org/packages/drupal/core once the parent issue is fixed :)
Comment #4
truls1502Comment #5
truls1502I have tagged on the wrong issue.
Comment #6
truls1502#3001735: Not able to download 8.3.1 with composer is fixed :)
Comment #8
truls1502It looks I have forgotten to add composer.lock file in the previous patch.
Comment #9
truls1502Comment #10
longwavephpcs
doesn't run properly with #8 as some of the sniffs have changed names.The attached patch renames these sniffs in phpcs.xml.dist and it seems to work after this, though running the full set against core reports a lot of "Opening statement of multi-line function call not indented correctly; expected X spaces but found Y", not sure if this is expected.
Comment #11
longwaveThis patch ignores the "PEAR.Functions.FunctionCallSignature.OpeningIndent" sniff which is new in PHP CodeSniffer 3.1.2, but is similar to PEAR.Functions.FunctionCallSignature.Indent in that we do not comply with it yet.
It also fixes a number of other new coding standards issues identified by the updated sniffs included in 3.1.2.
With this patch you should get clean output from a PHPCS run across all of core.
Comment #12
NickDickinsonWildeLooks good to me. Pretty minor changes to match the sniff adjustments so should be okay despite being slightly broader scope than ideal I think
Comment #13
alexpott+1 to being on the latest version of coder.
As a dependency upgrade that does more than just bug fix upgrades this will only be able to be part of 8.7.x - we need a reroll to apply to that branch.
One concern I had was that phpcs might have a higher minimum PHP dependency than Drupal 8s (current PHP 5.5) however looking at their composer.json file it shows that they are targeting PHP 5.4 so that's great.
I think we should have a change record because projects do track Drupal's dev dependencies. See https://github.com/webflo/drupal-core-require-dev
One thought is are we truly incompatible with coder 8.2? I don't think so if a project is stuck on 8.2 for whatever reason we could be kind and change the dependency to
^8.2.12|^8.3.1
Comment #14
longwaveI don't think we can be immediately compatible with both because of the renamed sniffs mentioned in #10, I assume the older phpcs will throw the opposite error with the updated phpcs.xml.dist file. It is possible there is some config or other changes we can do to work around this I guess.
Comment #15
alexpottThere's no re-roll necessary - the patch doesn't apply after
composer run drupal-phpunit-upgrade
@longwave you're right - we can't be compatible with both because
Drupal.Array.Array
=>Drupal.Arrays.Array
- that's debatable whether that should be in a minor version change - but semver is hard for contrib anyhow.Still I think it is fine for us to upgrade our dev dependencies like this in a minor version but tagging for needs release manager review for a second opinion.
I think a change record that documents the renames would be great.
Comment #16
NickDickinsonWildeDraft Change Record
Comment #17
longwaveOops I created a duplicate change record at https://www.drupal.org/node/3003759 but the other one is probably better.
Comment #18
alexpottThe CR looks good. I removed the extra one. We need an rtbc and a release manager +1.
Comment #19
jibranReroll after #2787871: Properly deprecate getUserName() and use getAccountName() instead. Setting it to RTBC as it is just a reroll.
Comment #20
alexpottI ran
composer run phpcs -- ./core -ps
locally after running composer install. All looks good.Comment #21
alexpottI tested running this patch on PHP5.5 for what it is worth and it runs just fine.
Comment #23
jibranComment #25
alexpottComment #26
catchWould slightly prefer the code-style fixes and the update to be in separate issues, although there's only a handful of changes so it's not too bad.
This doesn't apply to 8.7.x any more though.
Comment #27
jibran3 way merge FTW.
Comment #29
catchNeeds a re-roll. Fine with the small code-style clean-ups happening here.
Comment #30
jibranConflicts because of #2809503: Convert AJAX part of \Drupal\file\Tests\FileFieldWidgetTest to JavascriptTestBase.
Comment #31
alexpott#30 needs an update. It fails for the following reasons:
Re-rolled and fixed all the things reported by PHPCS using PHPCBF - this seems to be happening because #30 doesn't fully update PHPCS to latest version - which I think we should do here.
Since this is an automated tool setting back to rtbc.
Comment #32
jibranNice one my
phpcs -p --standard=core/phpcs.xml.dist core
was still running when I posted the patch :DComment #34
catchCommitted and pushed c67761090d to 8.8.x and b5a52c0814 to 8.7.x. Thanks!
Comment #37
heddnThis seems to have broken some contrib project's support for coder in drupalci:
See https://dispatcher.drupalci.org/job/drupal8_contrib_patches/53298/console
Comment #38
xjmThis is a disruptive change -- it also broke my local toolchain and prevented me from committing patches cleanly. :)
I resolved the issue with:
Do we have another issue for #37?
Also, any dependency update should be mentioned in the release notes. We should include as much information as possible about the problems sites might encounter and how to fix them.
Comment #39
catchComment #40
jibranDo we have another issue for #37?
As per #testing channel in slack @alexpott and @mixologic was looking into it. We have a purposed fix which can be deployed later today.Comment #41
jibran@catch added the release notes to IS and it looks good to me so marking it fix.
Comment #42
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedFollowing up heddn's comments in #37 this probably affects all contrib testing on drupalci. I have just checked Scheduler (which I maintain) and we get the same problem, see attached. This means that coding standards are not being checked on patches. As an example, I created a new patch in our old issue #2861902-26: Fix coding standards violations in 8.x codebase to purposely add some coding standards faults. The test output is clean with no violations, however, it should show:
So currently, patches could be committed to contrib which contain coding standards violations and this would be unseen by the tester and maintainer.
Comment #43
alexpott@jonathan1055 - @mixologic is aware and will fix asap.
Comment #44
MixologicA fix has been pushed to drupalci and now contrib modules should no longer get the conflict on installation.
I re-ran the test in #42 as a proof and it now properly gets the errors:
https://www.drupal.org/pift-ci-job/1242574
Comment #45
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks very much @alexpott and @mixologic. Yes coding standards checking on contrib is now being run again.
Comment #46
jibranPublished the change record after minor updates.