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
This is a follow-on from #3048498: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard where the automated patch fixed 653 of the 675 @deprecated messages reported under the Drupal.Commenting.Deprecated.IncorrectTextLayout
sniff.
Remaining tasks
Commit the first huge automated conversion chunkFollow-up with patches to fix the rest manually (Core 9.1 and 9.0)Enable the new Drupal.Commenting.Deprecated in phpcs.xml (Core 9.1 and 9.0)- Fix the rest manually (Core 8.9)
- Enable the new Drupal.Commenting.Deprecated in phpcs.xml (Core 8.9)
Release note snippet
The new Drupal.Commenting.Deprecated
rule has been enabled to ensure that @deprecated
documentation follows a standard format with relevant information for upgrading the code. See the deprecation policy for documentation of the standard.
Comment | File | Size | Author |
---|---|---|---|
#88 | 3094454-88.interdiff-83-to-88.txt | 3.94 KB | jonathan1055 |
#88 | 3094454-88.patch | 21.43 KB | jonathan1055 |
Comments
Comment #2
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedFirst patch to show the remaining warnings that need to be fixed manually.
Comment #3
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThat's the 22 that need fixing. Here's a patch with a few of them corrected, just to check that the mechanism is working as intended.
Comment #4
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedPatch to fix the full 22 remaining messages.
To help with finding the version I used
git show 55a4f988c1f
which gave #2854830: Move rest/serialization module's "link manager" services to HAL module andgit show bea23d7ce26
which gave #2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterfaceComment #5
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedWhilst making these manual changes, having the change record and/or issue link I thought I would add the missing @see links and missing extra-info. These will be required later when we fix
Drupal.Commenting.Deprecated.MissingExtraInfo
andDrupal.Commenting.Deprecated.MissingSeeTag
but given that these lines are being changed in this patch anyway, it would save future effort if they were fixed now.Source of info I used, in addition the the links in #4:
#2912169: Inject the argument resolver into HttpKernel::__construct
CR https://www.drupal.org/node/2959408
#2932462: Add EntityContextDefinition for the 80% use case
CR https://www.drupal.org/node/2976400
#2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API
CR https://www.drupal.org/node/2934779
Comment #6
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedSo that's all clean when adding the two new sniffs:
Here's the actual patch for review and commit, same as #5 but with no changes to drupalci.yml or phpcs.xml.dist
Comment #8
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAll of the failures in #6 have the deprecation message
It seeems that due to luck and/or a quirk of how the @deprecated tag was added to WorkflowDeleteAccessCheck that core tests should have been throwing this deprecation message all along. Fixing the standard has alerted us to this.
Patch #8 is the same as #6 but with the single change to WorkflowDeleteAccessCheck removed.
Comment #9
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented#3009848: Fully deprecate WorkflowDeleteAccessCheck is done so now these last few @deprecated text layouts can be fixed. First, check to see the remaining problems.
Comment #10
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedPatch #9 shows 22 @deprecated faults over 19 files. We had 22 before the WorkflowDeleteAccessCheck was fixed, so only one new bad @depreated basic layout was committed to core since November. That matches with the changes I had before + one new one. Patch #10 fixes those 22.
Comment #11
Gábor HojtsyWoot, thanks!
Why do you need the host command?
We would need a patch without the drupalci.yml file changes to commit. I think we should commit to Drupal 9 for sure, but for consistency and easier deprecated API detection (eg. actionability), we should commit to Drupal 8.9 as well.
Comment #12
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThe host command is to patch Coder because my final changes are not yet in a tagged release. I discovered early on in the analysis that lots of the @deprecated annotations in Core are correct (or could be automatically corrected) but do not have any 'extra info'. So one of the enhancements is to cater for this in a separate sniff so that the main text layout can pass. This was in place for all the work on #3048498: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard
So the 22 corrections could be committed now, but we cannot turn on the actual sniff until (a) patch #16 on #3057988-16: Automatically fixing @deprecated doc text has been committed to Coder, (b) Coder has new tagged release, and (c) we confirm that Core uses that new release.
Comment #13
Gábor HojtsyCan the coder fix be committed first, so we can commit the fixes for deprecation messages and the coder rule enablement at the same time? Or is that not on the horizon?
Comment #14
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedIt is ready now, as far as I am concerned, and I would be happy for that to be committed any time. Needs @klausi or @pfrenssen to review and commit, and ask them the timescale for the next Coder release.
Comment #15
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedJust to update this thread, @klausi reviewed my work on https://github.com/pfrenssen/coder/pull/90 and I have addressed his points. Now waiting for re-review.
Comment #16
Gábor Hojtsy@jonathan1055: Drupal 9.0.x does not have much @deprecated anymore, a handful are left for Drupal 10. It would be the very best time now to turn on the rules in Drupal 9 before more non-conformant ones are added :)
Comment #17
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedYes, that sounds good. Here is patch which will show all the @deprecated and @trigger_error coding standards messages, to see what we get on 9.0
Comment #18
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented35 messages in total, but we have not been fixing the @trigger_error messages yet in 8.9. So here is just the @deprecated sniff.
Comment #19
Gábor HojtsyThat only has two woot :)
Comment #20
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThis patch will fix those two messages, and also not stop short, but will run all the tests.
Comment #22
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedOf course, the test for the expected deprecation message also has to be amended. I can't run local tests on D9 yet.
Comment #23
longwaveRerolled,
update_fix_compatibility()
is gone so we can just enable the standard and fix the deprecation test module.Comment #24
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks longwave. Yes, to make sure of this, I've re-run patch #18 and it now shows not two but just the the one deprecation mesaage, in the test file which is fixed by patch #23
So #23 is RTBC. What other users need to OK this before it can be committed to 9.0.x?
Comment #25
longwaveWe just need to wait for a core committer to look at this now it is RTBC.
Comment #26
xjmThis has a (soft) RC deadline.
Comment #27
xjm(Wrong issue.)
Comment #28
xjmAlthough enabling new rules also typically has an RC deadline, so adding back the tag. :)
Comment #29
xjmComment #32
xjmCommitted to 9.1.x and 9.0.x. Thanks! It's great to finally see the standard adopted and will make the D10 upgrade even easier.
Normally we would also backport the new coding standard to 8.9.x, but in this case there are probably hundreds of issues missing change record links in 8.9 and 8.8, so we can't yet. There's still a meta issue open to add CRs to those old deprecations, but this will ensure the standard is always followed going forward.
Thanks!
Comment #33
xjmWe list newly enabled coding standards in the release notes, also.
Comment #34
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks @xjm for committing to 9.0 and 9.1.
Well, there were 675 in 8.9.x but I managed to fix 653 of these basic layout problems automatically in #3048498: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard. There are now only 22 as per comment #9 above. This issue started at 8.9 but Gábor moved it on to 9.0 to get the standard in first. I will re-check/re-roll that patch and see where we are in 8.9
Comment #35
xjmPTBP then?
Comment #36
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedYes. But was I too fast in changing the version? Should it remain at 9.0 for the Change Record and Release Notes. I can still work on the patch anyway.
Comment #37
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThe patch will be bigger for 8.9. Going to work on this. Did not intend to set the status back to fixed in #36.
Comment #38
xjmI don't think we need a CR, although yeah a release note would be good since we need to say something.
Comment #40
alexpottWhilst committing another patch I ran into
#23 should have been done against 9.1.x - but this is the only instance so I've hotfixed it as changing
in
tofrom
feels very minor.Comment #42
pameeela CreditAttribution: pameeela commented@jonathan1055 we're preparing the release notes, can you update the IS with a release note snippet? Just a brief summary of the change with a link to additional documentation if needed.
Comment #43
xjmComment #44
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi @pameela,
The issue where the standards were agreed is #3024461: Adopt consistent deprecation format for core and contrib deprecation messages. Even though that issue summary says "proposed" standard, it has been agreed in that thread, and completed in Coder. See also https://www.drupal.org/core/deprecation
I have updated this IS with the standard and example. and split the tasks over the D9 and D8.9 version for clarity. Hopefully that is what you wanted?
Comment #45
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedNow we are back with Core 8.9. This new patch #45 is essentially the same as #10 (which still applies). However, now that Coder 8.3.9 has been released we can ue that instead of applying the patch to Coder 8.3.8. Hopefully this change to drupalci.yml will work:
Comment #46
longwaveBuild failure is:
Comment #47
jungleBTW, the failure in #45 might be related to the bug that coder 8.3.9 got installed to module/contribute.
Comment #48
jungleComment #49
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@jungle how did you see that coder was installed at
module/contribute
?So we have
and
and we also have in the phpcs output
which does imply that the Drupal and DrupalPractice standards are not available.
I added
cd ${SOURCE_DIR}
but was that not the right place to attempt to upgrade coder?Comment #50
jungleJust guessing. I encountered that error before due to the reason i mentioned. Anyway, coder 8.3.9 was in, let's revert the DrupalCI customization?
Comment #51
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedYes, Coder 8.3.9 release was requested to allow my changes to be available for these issues. Great that @klausi and @alexpott committed those so quickly.
Here is patch #41 but without the attempt in drupalci to upgrade Coder.
Comment #52
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAll clean, so this patch #52 should be the actual final patch for committing to core 8.9. This will enable the
Drupal.Commenting.Deprecated
rule and use sniffDrupal.Commenting.Deprecated.IncorrectTextLayout
but exclude the other 5 sniffs in this standard.Comment #53
xjmThanks @jonathan1055. We don't need to re-document the coding standard in the release note; we just need to say that the standard is now enabled for the core ruleset. I think we could just say that deprecation messages must now follow the documented format and link the handbook page for the example.
You can check past minor release notes for an example of the release notes about enabled coding standards rules. (I don't recall offhand which might have an example, but check 8.8.0, 8.7.0, etc.).
Comment #54
xjmUpdated with a release note that explains the purpose of the rule and links the deprecation documentation.
Comment #55
jungle#52 LGTM, no CS violations, thanks!
Comment #56
xjmThanks -- mostly just some small formatting feedback.
All of these are wrapping too early on the second line, I think.
Over 80 chars.
Was there also never a change record? If so, should we fix that?
Also over 80 chars. (It was before also, but that doesn't mean we should persist with that when re-wrapping.)
Comment #57
jyotimishra-developer CreditAttribution: jyotimishra-developer commentedComment #58
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi jyotimishra123,
I am 3/4 way through fixing this. How much have you done yet? Sorry I did not assign it to me, as I had been the only one working on this patch, so just assumed no one else would do it.
[edit: Argh, did not mean to unassign, it was stale form data. If you want to do the work, let me know here]
Comment #59
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commented@jonathan1055, working on this.
Comment #60
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedsja112? OK, even though I have got it nearly all done. Just for info, and to save you some effor, for (2) you can have
because otherwise the next line will extent beyond 80 but you can't fix that as the @link @endlink has to be in the same line. You can update the trigger_error text to match this change, or it can be done separately, as there is another issue for that.
And also don't fix (3) as that is not in scope - the sniff
Drupal.Commenting.Deprecated.DeprecatedMissingSeeTag
will be covered in a separate issue, we are only fixing the mainDrupal.Commenting.Deprecated.IncorrectTextLayout
sniff here and enabling the rule.Comment #61
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedjonathan1055, Thanks for the info. I will update the patch accordingly.
Comment #62
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commented@xjm, Updated the patch as per the review comments.
As per comment #60,
haven't worked on #56.3
Thanks!
Comment #63
jungleThanks @sja112!
Exceed 80 chars.
Comment #64
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commented@jungle, Thanks for the review.
I have updated the patch as per your review comment.
Comment #65
jungle@sja112, thank for your quick response!
The lines after
a context definition for an entity type (i.e., the data type
should be adjusted accordingly. for instance, movingbegin
orbegin with
to the previous line, as long as it does not exceed 80 chars.Tip: you can cancel the CI run on the CI job page, for instance: https://www.drupal.org/pift-ci-job/1692122
Comment #66
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedSecond tip, as we are only running coding standards, you can use the change to drupalci.yml in the top of my patch 51 https://www.drupal.org/files/issues/2020-05-13/3094454-51.fix_%40depreca... so that the run ends very quickly, saving resource. Then when all the changes are agreed, remove that and run one final time.
[edit: stale form data again! did not mean to set to needs review, but it will be needs review soon, so no point changing it back now]
Comment #67
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedAs per #65 and #66, I have updated the patch.
Comment #68
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedIn #67 haven't received any error related to character count. Reverting the changes and adding the final patch for review.
Thanks once again @jonathan1055 for help.
Comment #69
jungleMore needs adjusting. Well, assign to myself.
Comment #70
jungleAddressing #69 and other small changes, details coming in #71
Comment #71
jungleRemoved redundant "can"
Added a prefix \
Changes in #70
Comment #72
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThe actual sniff for character count is ignored for drupal core as there are too many files that fail this rule.
Comment #73
jungle> #56.3 Was there also never a change record? If so, should we fix that?
No change record. Found that #3098814: Class 'Drupal\Core\Controller\ArgumentResolver\RawParameterValueResolver' not found during update to 8.8.0 introduced it.
The coming patch added a @see. So #70 or this one. Up to you :)
No testing triggered on purpose.
Comment #74
jungleIn addition to #72, as the NW request #56 was from a committer -- @xjm, so to me, even though this kind of changes are unnecessary, I am intending to address it. Similar message was delivered to me by @alexpott as committer, somewhere in the issue queue i could not remember.
Comment #75
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks @jungle and @sja112.
Well, I was just trying to not let the scope creep, but as you've done the work and actually found a link, let's keep it.
Two things to query in patch #72
1.
Is this really allowed? Can that @link be rendered in IDEs when it spans over a new line to the @endlink. Maybe it is OK?
2.
My suggested change in core/lib/Drupal/Component/Utility/SafeMarkup.php is good
However, to make it easier for someone in the future, we might as well make the same change in the @trigger_error text, as the standard is to have the same wording. It needs the following change:
There will be other @trigger_error messages which fail the standard, but those are for a separate issue. This one should be fixed here because we have actually changed the message wording very slightly to assist spanning and @link.
Comment #76
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commented@jonathan1055,
Thanks for your input.
As per your comment #75.2 I have updated the patch.
Also includes fix for reoccurrence of word
sanitization
twice.Comment #77
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedComment #78
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedNo, that's wrong. The word is needed twice. It is inside the
@link
, the first is the url, then the remaining is the text. See api-documentation-and-comment-standards#linkAlso, as suggested in #75.2 if that line has to be changed it should be fixed for @trigger_error standard too, to save later work. I gave the actual text for that "SafeMarkup::checkPlain() is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0." in that snippet.
Comment #79
jungleAddressing #75, @jonathan1055, thanks for your review!
Searched code in core, found no example of @link ... @endlink spanning more than one line. so adjusted again to make it one-line.
Comment #81
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedOK so if we fix that message, we also need to fix the test.
Or do neither and leave it for the other issue.
Comment #83
jungleFixing the test.
Comment #84
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedNice work @jungle (and @sja112 before).
Thanks for adjusting the expected deprecation in the test, better to do that here as we are looking at these files and will save someone's effort in the future.
That's right, and this also explicitly stated in api-documentation-and-comment-standards#link
Everthing in #56, #63, #65, #69, #71, #75, #78 and #81 are fixed so marking patch #83 as RTBC to allow core reviewer to follow up.
Comment #85
xjm🤔 Why are we changing the grammar here? The sentence is no longer parallel.
Is this because the escaped single quote is causing a false positive on a rule violation? If so, could we just convert the string to double quotes instead of changing the grammar?
Comment #86
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThe @deprecated change was
which meant we also needed to keep the @trigger_error text matching it. It was your line-length > 80 query on this line which meant we had to change the @deprecated first line. But if it wrapped instead of shortening then the next line @link ... @endlink would extend beyond 80. However, I have now learned that @link .. @endlink syntax is allowed to extend beyond 80, so yes this grammar change could be reverted and the wrapping adjusted instead.
Comment #87
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAssigning.
I will re-do that change and revert the @trigger_error change
Comment #88
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThe @link ... @endlink is only allowed to extend beyond 80 when the entire link is bigger than 80. Having it extend beyond 80 when it can be moved to the next line is what the standard requires, hence the re-wrapping of the whole paragraph.
Comment #89
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commented@jonathan1055,
Thanks for the updated patch.
The @trigger_error, comment and test case changes are reverted.
#85 is addressed so marking patch #88 as RTBC.
Comment #90
xjmOops, sorry for not being clear. We can keep the standard:
...in drupal:8.0.0 and is removed from drupal:9.0.0
. It's just more grammatically correct to say "Rely on Twig's auto-escaping feature, or use..." because then both parts start with an imperative verb.There's also an interesting problem with the
@link
/@endlink
stuff. On the SafeMarkup API documentation they link the correct documentation topics; however, they won't work in the@trigger_error()
and so developers who find the message that way will have no information about what they're supposed to do.But! I realized now all of that is out of scope, because this issue is just fixing the
@deprecated
, not@trigger_error()
. We can file a followup to fixcheckPlain()
's messages somehow.Meanwhile, removing the rest.
Comment #91
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi xjm, thanks for the explanation and review.
Yes I have done. That is in patch #88
Yes I agree. That is what I reverted to, that is what patch #88 has.
No sure what this means?
Good spot with the @link in the @trigger_error. There may be several of those and yes they need to be fixed in another issue.
So ... what 'needs work' is required on patch #88?
Comment #92
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedSetting this to RTBC for patch #88 as I think the issue was set to 'Needs Work' unintentionally. In #89 it was marked RTBC and I have answered all the quesitons in #90. The patch still appiles and passes all green. It would be good to get this in and committed to 8.9 which will enable the basic @deprecated standard check.
We have a release note - see #54 so the addition of "missing change record" from #29 may not be required anymore.
If we delay much longer then the benefit will be lost. We've been so close since xjm's review in #56 but some noise, mis-understandings and out-of-scope change have obscurred the progress (I have been the culprit here, too).
Comment #93
xjm@jonathan1055 Yeah sorry about that; that comment was posted exactly to the minute when we started publishing Wednesday's security release. I think I clicked "save" without meaning to at the time, and then afterward did not have a chance to revisit the tab. I think what I was trying to say was that committing the rest was not possible at the time due to the code freeze, and that it was TBD whether we could still make the change in 8.9. Which we for sure can't now. :) So marking fixed against 9.0.x, but saving credits for the work on the backport too. Thanks!
Comment #94
xjmComment #95
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAre you really sure this can't be committed to 8.9? It is not making any change to a single line of active code, only @deprecated doc commenting. The whole point of this is to get the standard sniff enabled on D8.9 to make it easier for code scans, etc. If this is not done it renders the entire standard menainglesss if not enforced.
Comment #96
xjm@jonathan1055, 8.9 is now a production branch and is not allowed to have any changes to deprecations anymore. New deprecations will only go into 9.1.x, so that's where it's important to have the rule enabled -- and we do! Finally! So that is good news. :)
Both enabling a new phpcs rule and changing a deprecation message in any way are disruptions that aren't allowed from RC on, but on top of that, we aren't going to ever add new deprecations to 8.9. So even if we broke two separate rules of our allowed changes policy to commit this patch, we wouldn't really be getting any future benefit out of it, since there won't be any new deprecations we need to check.
I should have made it clear earlier that we were racing against the clock a bit, sorry. That's what the "rc deadline" tag that I added back in the end of April meant: This issue had a deadline of the code freeze of the first release candidate for each branch it was committed to. That originally would have been May 4 or so for both 9.0 and 8.9. We postponed it because we didn't want to release the first release candidate without SA-CORE-2020-003 and a few other things being resolved. 8.9.0-beta3 was released May 17, SA-CORE-2020-003 finally went out on May 20, and the 9.0 and 8.9 RCs were released on May 22, and so we were in code freeze most of that week for all those releases.
For more information, see:
Hope this helps! TLDR it's too late for 8.9, but we also didn't totally need it in 8.9 anyway. It was just a nice-to-have to fix up the inconsistent messages in our LTS branch, but it's OK also that we didn't get there before the branch went into RC.
Comment #97
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks for the info and for explaining it. I did feel very deflated when I discovered it was not going to be committed in 8.9, having put so much effort and so many hours of my free time first writing the sniffs in Coder then working on the automated fixes, then these final few manual ones. (I am not sponsored, and not employed to do this, everything is my own time, like many here I know). I suppose you could say it was my fault not to have known what the 'rc deadline' tag date was. In your review on 19th May "mostly just some small formatting feedback" if you had mentioned the deadline I would have worked to meet it, but instead I encouraged the contributions of others, as is the Drupal way, and let people who wanted to help learn. There was lots of wasted time, but it did not seem to matter as people were gaining experience. Had I known the deadline I would have got the patch ready for re-review immediately. But ... that's all history now.
At least we have this in 9.0 and 9.1 so our efforts were not wasted. Right, now on to #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard
Comment #98
xjmYeah @jonathan1055 I totally understand -- this sniff has literally taken years for us to adopt. And it can be disappointing when extra work doesn't get used, like we had for those past couple weeks. That's part of why I made sure everyone who worked on the backport still received credit even though the backport wasn't actually committed at the end of the day. Thanks for your understanding and commitment to this chain of issues!
Comment #99
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThank you :-)
Interesting that you said on 25th May in #93 above that no more changes could be made to core 8.9 but on #3138591: [D8 only] Add missing E_USER_DEPRECATED to deprecation notices there are commits to 8.9 yesterday, 28th May. The priority of that issue is only 'normal' and it is changing actual active code
@trigger_error(... , E_USER_DEPRECATED)
.I have read https://www.drupal.org/core/d8-allowed-changes but I'm confused now about what is and what is not allowed to be changed.
Comment #100
xjm@jonathan1055 Yep, those were wrong changes that I also noticed just now when curating issues for the release notes (that issue and about 3 others also). We've actually just restored the rulesets from 9.0.0-rc1 and 8.9.0-rc1 because of this:
https://git.drupalcode.org/project/drupal/-/commit/6144e3b1f69b5ff9c786c...
https://git.drupalcode.org/project/drupal/-/commit/2a5cd8c8adcdae777324d...
Comment #102
pameeela CreditAttribution: pameeela commentedComment #103
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@pameeela Why add the tag to this issue which was fixed 6 weeks ago?
Comment #104
pameeela CreditAttribution: pameeela commentedAh sorry, this was a mistake. We’re using the tag to track initiative stats so I updated tickets I worked on, but this was pre-bug smash so wasn’t one of them. Removed! Thanks for raising it.
Comment #105
quietone CreditAttribution: quietone commentedA followup was asked for in #90 concerning the use of @link/@endlink in the trigger error message. That message was removed from core in Drupal 9.0, which is no longer supported. Therefore, removing the tag.