Tweaked the release note a bit to clarify that it's a removed dep, and added to the draft.
Thanks!
And a CR.
We need a release note for this.
Per @longwave, 📌 [jQuery 4] jquery-form is unmaintained and not jQuery 4 compatible, fork it into core Fixed has removed this from the critical path, so downgrading to major.
Based on @fgm's comment on the parent issue, I think this is critical.
Adding #3444792: Prepare for PHPUnit 10 → to the IS explicitly since it was reported that we broke GitLab CI templates.
Also fixing parenting.
Oops, fixing branch.
I updated https://www.drupal.org/docs/getting-started/system-requirements/php-requ... → (although it was more in the scope of branching 10.3 than this issue, since it's about supported versions).
We could add a CR about the recommendation bump and performance impacts etc., so that we could link it from the page, but it looks like we didn't do that for 10.2 either so probably the release note and core UI message are sufficient.
Thanks everyone!
Also properly crediting @longwave for the commit, since d.o ate his credit.
Adding credit for @longwave who gave feedback on the change in Slack.
10.4 first.
xjm → created an issue. See original summary → .
Saving credits.
We decided not to create an alpha1, so transferring credits from this issue to the beta1 draft.
Transfering credit for @catch since we decided not to do a 10.3.0-alpha1 and he had worked on that draft, which feeds into this one.
Adding credits for two sponsors that both sponsored part of my time.
pameeela → credited xjm → .
pameeela → credited xjm → .
(Oh, I forgot to mention that previously we would have included this in the release notes, but as we've added more and more rules, we've moved away from enumerating them all the time since the tools tell you which they are.)
Committed live! at DrupalCon Portland 2024 to 11.x, 11.0.x, 10.4.x, and 10.3.x. It did not cherry-pick cleanly to 10.2.x. Setting PTBP for a 10.2.x MR.
Thanks everyone!
And hiding screenshot. (We only need screenshots when there is a user-facing change to evaluate.) Thanks!
For #32.
Committed to 10.4.x and cherry-picked to 10.3.x. (Hopefully I did not break 10.3...)
Thanks!
(Branch sorting is slightly weird now that 11.x is open.)
Saving other credits as well.
Crediting usability meeting participants.
There are two different patterns here that we are fixing -- comparing to NULL and using isset() -- but the number of changes is small enough that it's sensible to fix both in a single issue.
Enabling new rules is a good alpha-ish-beta-ish target, so I've committed the issue to 11.x and cherry-picked it to 11.0.x. It did not cherry-pick cleanly to 10.4.x, so we need an updated version for the D10 backport.
Thanks!
Checked and confirmed that this method does not exist in PHPUnit 9, so this is an 11.x-only change.
Committed to 11.x. Thanks!
That said, removing the out-of-scope changes will be a good novice task. I recommend using git diff --color-words to more easily spot the out-of-scope changes.
This definitely should not be postponed on the other issues; rather, this should go in first.
There are still out-of-scope changes per #62. For example, see the change in core/modules/content_moderation/content_moderation.module and core/modules/system/src/Controller/AssetControllerBase.php.
There is also still a bad URL change in core/modules/file/tests/src/Functional/MultipleFileUploadTest.php and in other places.
core/modules/locale/src/PluralFormula.php is adding a grammatically incomplete sentence to fill out the @todo rule.
Both adding test coverage for the expected exception and fixing #10 could be good novice contributor tasks.
This is also failing static analysis:
------ ------------------------------------------------------------------------
Line core/lib/Drupal/Component/Plugin/Discovery/AttributeClassDiscovery.php
------ ------------------------------------------------------------------------
131 Instantiated class
Drupal\Component\Plugin\Discovery\InvalidPluginDefinitionException
not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
------ ------------------------------------------------------------------------ An outstanding task in the issue summary is to look into why these changed location between Stable and Stable 9 to begin with.
Also, despite #8, I'm not totally convinced the impact of this outweighs the disruption. The stable base themes in general are not supposed to change except between major releases. If we do decide that the disruption is merited, it would probably need to go in the release notes.
A novice contributor could do the git and issue archaeology to look into why the templates ended up in this location. Maybe that can help inform the release and frontend framework manager decisions about why the change was made and whether changing it back is an allowable change in a minor version. Thanks!
Given that we have three experienced contributors to CKE5 disagreeing about the approach I think this is not really in a novice state. 😀 Thanks!
I'd say this isn't really currently in a novice state. Thanks!
We now have two different possible approaches, one in a merge request and one in a patch file. In general, we need to use MRs from now on because patches will no longer work with automated testing. Without doing more research, I'm also not totally sure that either approach is quite right here, so untagging as novice for now. Thanks!
Oopsie, did not mean to remove the event tag.
We should probably have @ckrina or another subsystem maintainer sign off on the icon choice and the changeset. Thanks!
Please also be sure to hide the non-canonical merge requests. Thanks!
Given how long it's been since there was activity on this issue and the ongoing discussion about the best approach, it is probably not a good novice issue choice. Thanks!
Given the extent of my issue update in #27 and the fact that it was over seven years ago, this issue probably isn't a good novice contribution candidate. Thanks!
Based on the numerous different approaches that have been tried, I think this isn't really in a novice task state ATM. Thanks!
Updating the remaining tasks to point to @Wim Leers' feedback. Thanks!
Updating the remaining tasks section.
To be clear, the task is to add test coverage to the MR (a test that fails before the fix and passes with it). Thanks!
This does seem like a good issue for novice contributors to investigate (with regard to why the test is currently failing). One helpful step would also be to manually test a site with the test's setup and validate that it is actually working correctly.
Additionally, I left a note on the MR to remove the old inline comment.
Please also hide the non-canonical MR and check to ensure that the canonical one is mergeable with pipelines passing. Thanks!
Agreed, thanks @joachim!
There is also a coding standards issue to resolve:
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
16 | ERROR | [x] Opening brace should be on the same line as the declaration
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
Time: 16.64 secs; Memory: 8MB
PHP CODE SNIFFER REPORT SUMMARY
-------------------------------------------------------------------------------Actually, I'm removing the novice tag from this issue -- given the recent discussion, this is not currntly in a state where further screenshots or manual testing are helpful; none of the remaining tasks as listed in the issue summary are really novice when we're stuck in a cycle of trying to fix one bug without introducing a different one. Thanks!
People clicking the MR suggestions accept button without letting issue authors see them is becoming a problem.
This needs a CR. There can be a single CR for all the D11 DB requirements.
Shouldn't this have a change record? We can have a single change record for all the DB version requirements for D11.
@pradhumanjain2311, you are not a release manager and therefore have no place in this issue. Don't create issue forks for random issues. This is credit gaming behavior and creates a burden for committers and other contributors.
That's the one.
Crossposted with the 11.x revert.
Better status I guess; it needs to be built for the branch rather than cherry-picked.
Nice work on this!
Posted a variety of nitpicks and one serious recommendation for a bit of a refactor (i.e.: Get rid of the default values and require user input for the commit hash).
I did not manually test the script.
I also just noticed no one actually ever answered @alexpott's question in #50 and #51. NW for that also.
FWIW, I think there is a usecase here, but we should articulate specifically why this is the right approach (vs. removing the link in this case).
If we need to escalate it for further feedback once that reply is given, I think it should go to the usability team and/or comment subsystem maintainers; this is not really a product-level decision. Thanks!
Saving issue credits.
Thanks for your work on this issue! Thanks especially @pameeela for the very clear manual testing. I've updated the issue summary with those images.
There are a few coding standards and best practices issues with the current MR (see my comments therein). I do understand that in some cases these are being copied from existing tests and/or surrounding code, but we should not do the wrong thing for the sake of consistency. (If desired, a followup can be filed to improve the best practices in the rest of these tests.)
I think this is pretty close.
For future reference, "Remove deprecated site settings" is a good issue scope since it requires a different kind of research than most other removals. The removal from System is also related because it's about converting system configuration to settings.php.
I confirmed that there are no remaining references to block_interest_cohort or yaml_parser_class
Nice work on updating the test to use an 11.x example.
Committed to 11.x. Thanks everyone!
I also don't see any record of the usability review happening. @TomiMikola, please leave it up to the usability team and maintainers as to whether a usability review is required. Issues should not be marked RTBC when a usability review has been requested but has not happened yet.
In order to prepare for the usability team reviewing this, the MR should be gotten into a state where it applies and passes tests, and the issue summary should be updated with screenshots of the current "before" and "after" (both of which should be updated to show 11.x HEAD, since this issue is 5 years old).
Finally, we should investigate the relationship between this issue and the one @quietone linked.
Thanks everyone!
The MR is failing tests and not mergeable. Thanks!
@smustgrave, again, I don't need validation that the suggestions were applied -- I can see that from the tooling -- I need validation that they were good suggestions. Thanks! :)
Aside: This is the issue to tag for both release notes and release highlights, if/when the FEFM signoff is given.
Tagging per my comment above about the temporary label and machine name being generated to make the form work.
I finally finished my manual testing and my code review -- phew! Overall this is in pretty good shape, and it's a really good usability improvement on the whole. IMO there are three regressions that need addressing (see #47, #49, and #51), plus a few small cleanups, but on the whole it's close to ready. Great work everyone!
Saving issue credits.
Another regression is in the page title for the field configuration page. In HEAD we have:
With the patch it's simply:
This sounds like it's all field settings for articles, rather than the settings of one particular field.
I understand that we don't have the field name when the page is loaded, but could we at least put the field type in the title? E.g.:
Boolean field settings for Article
I confirmed what @smustgrave said above: If you click out of the label field, the bug does not happen. (Clicking in or out of the label field in HEAD had no effect; it worked every time.)
I also reproduced #47 in Firefox, both it not happening in HEAD, and it happening with the patch applied.