Tagging for the Nightwatch warning followup and adding it to the IS.
FWIW I got the Nightwatch warning in #25 on HEAD when I was prepping for the live commit at Singapore. Asked @larowlan about it and he suggested ignoring that at the time.
longwave → credited xjm → .
Thanks @pooja_sharma for working on this!
@larowlan and I discussed this in person at DrupalCon Singapore and agreed that the best approach is to leave the cache invalidation for now, but to have a followup issue to see why it's needed and maybe fix it.
The specific format in the MR currently needs a little work. It should be an inline comment containing a @todo comment and a link to a new followup issue. Here are a couple references that should be helpful:
Hope those help. It was great to meet you in person!
Out of curiosity, I grepped to see anywhere else that is-super-user is interacted with like this and found ./core/tests/Drupal/Tests/Core/Session/SuperUserAccessPolicyTest.php, which makes sense.
Meanwhile. The IS says that core/tests/Drupal/KernelTests/Core/Render/RenderCacheTest.php should be removed. However, the MR does not remove it. Should it be removed? I'm a little confused -- that's an awfully generic test name for the scope of this issue. Could we explain further?
Committed live at DrupalCon Singapore to 11.x, 11.1.x, 11.0.x, 10.5.x, 10.4.x, and 10.3.x!
Sounds like this series of issues had a journey through several Asia/Pacific events. Thanks everyone for modernizing our testing infrastructure!
Adding credit for security team members who discussed the private issue related to this during the fortnightly core security triage as well as a duplicate of it.
We agree with the public handling, and simply removing this brittle code that is incorrectly coupled to the old special behavior of user 1 seems the best course.
longwave → credited xjm → .
quietone → credited xjm → .
.
longwave → credited xjm → .
smustgrave → credited xjm → .
smustgrave → credited xjm → .
pameeela → credited xjm → .
Remove unnecessary step about updating GL default branch now that we have 11.x pseudo-main.
Thanks @catch I've updated the issue to reflect the consensus around not having a new config option for this.
Huh? When did that get consensus? I still don't agree. I suggested a config-only option, not adding it to the UI. And I still feel this is too disruptive for a minor release because your sites start freaking sending real people emails when you create accounts. Spam by accident is much worse than no spam by accident.
gábor hojtsy → credited xjm → .
gábor hojtsy → credited xjm → .
There is least one person missing from the list, will ask in Slack...
Release management hat on: Changing the default value on the form would be too disruptive. I would say that's a major-only behavior change.
I think Starshot can flip the default value from core's, and which way the default behavior goes can at least be added as a site-wide config setting. That way, Starshot and site owners could override it. I think this is potentially something where the Starshot and core framework 80% case differ.
I'd still also lean toward exposing the site-wide default in the user settings admin UI, but we can separate out that decision to a followup as well if we want. That also would give us the opportunity to deprecate a checkbox on a more frequently used form, if we want.
@lauriii raised the question of whether this could be tied to the dev/prod toggle. I don't think it should be, because the usecase @pameeela has raised is to notify real new users via email when an account is created, but mine (from my own site-building days) is always to not notify users created by admin.
We could also use the dev/prod toggle to not send any emails from dev ever, but that's its own separate followup issue.
If anything (if we want fewer settings in the UI), I would almost make it only a site-wide checkbox, rather than a checkbox on every user creation form. That would more align with core-as-framework IMO. But that also seems like followup material to me.
larowlan → credited xjm → .
Adding @quietone to the credits since she also contributed to the ongoing RM discussions about this issue.
We had previously discussed for this issue that we would not backport this update to 10.2 unless CKSource could provide us a non-standard release on the old major that 10.2 was using, because the changes in CKEditor 5 v41 that were released in Drupal 10.3 broke a number of contributed modules. So this should only be backported to 10.2 if it also comes with a backport to CKEditor 40.
The title of this page does not seem to exactly match its contents; should it be "Configuring maintainer access" or something?
Just hiding the old patch files from the issue summary to make the current target clearer. :)
Re-crediting myself since the System Message ate it. :)
Committed to 11.x. I've also backported the issue to 11.0.x, 10.4.x, and 10.3.x, because API documentation improvements are allowed in production bugfix (patch) releases of core.
Thanks everyone! 🦎🌊🎉
Thank you everyone for working on this issue!
We reviewed this issue LIVE at DrupalCon Barcelona 2024 at the mentored contribution sprint.
-
After reviewing the proposal, I think @joachim's proposal is a good one. I looked up the detailed documentation for QueryInterface::condition(), and it does indeed explain the needed information much more clearly and in greater detail.
-
I also looked at @vinmayiswamy's suggestion in #6. Thanks for thinking about this question! In this case, I think the linked
QueryInterface::condition()documentation also sufficiently covers that information. -
Finally, I considered @smustgrave's suggestion in #7:
Seems the updates were slightly copied and paste from the summary, but don't think we should be introducing new phrase that don't already exist.
For comprehensive documentation on the
is not found in core. Example
* See the @link themeable Default theme implementations topic @endlink for
* details.That's just a suggestion, that may fit but should checkout other examples in the repo so we can stay more inline.
This is also a good line of thinking! In this particular case, though, I think we should keep the wording that's in the current merge request. It's always helpful to have information for someone about why they might want to follow a link, so that they can decide whether or not it is helpful for them to view it.
Adding credit for @bibliophileaxe who helped mentor the contributors who worked on this issue today.
longwave → credited xjm → .
@larowlan Trying to get decisions documented in the related issues of the children.
Filing followup issues after carefully reading the issue to understand what to file is a good novice task still, so someone could do that on contribution day.
#51 still needs to be addressed. When someone is able to address it, this issue can be converted to a merge request (but credit will not be granted for converting this issue to an MR without addressing #51). Thanks!
Oops, Gábor left a bit before others and I forgot to add him back. :)
The list of completed issues in the IS makes this page very difficult to load and manage, so removing it from the IS. Sorry. :)
@catch, @longwave, @alexpott, @lauriii, @bnjmnm, and I discussed this issue at DrupalCon Barcelona 2024.
Previously on this issue, we discussed the possibility of allowing two values, three values, or an arbitrary number of user-defined values for different site environments.
Today we discussed and agreed that we will restrict this to two modes (dev and prod, or along those lines), for the following reasons:
-
"Staging" or "QA" means different things to different sites and organizations. If we provide this middle option, developers and site owners will make assumptions about what it means in terms of which production behaviors you get. (E.g., that a site in staging won't send email, or that it will but that the mail client will handle the test emails, etc.). These misunderstandings could have serious consequences for site data and operations.
-
It's comparatively easy for a site owner or developer to understand what a site being "dev" means and how that differs from production.
We also discussed where this flag should be stored:
- It should not be in config because it is not something that you deploy.
- It could be stored in either
settings.phporservices.yml - We could optionally allow overriding it in state or similar (so that there would be both filesystem and database methods for overriding it).
#75 in particular is out of scope for the dev/prod toggle we're discussing here.
@catch, @lauriii, @Gábor Hojtsy, @poker10, and I discussed at length how we can move this issue forward.
-
Our current focus and priority is to get this committed as an alpha module. To this end, we will do thorough reviews, but only block committing these issues on absolutely critical issues that must be fixed before the issues can be committed (e.g. issues that would have serious negative impact on contribution experience). As a reminder, with alpha-stability modules, open critical issues are not resolved before a new minor release is tagged, the modules are simply removed from the release branch.
-
We are aware of the downsides of making the MR the source of truth instead of the contrib module, but we've gotten to a point where this is significantly slowing contribution to the MR, so it might be time to make the MR the primary source of truth.
-
Blocker: 📌 Package manager/ Automatic Updates should disallow composer patches by default Active
-
Blocker: https://github.com/php-tuf/composer-stager/issues/78
For this issue, the release managers think that most if not all of the remaining dev dependencies could also be removed. The original issue summary is out of date (e.g., Infection has already been removed). A contributor could help by updating this issue to reflect the current status, so that release and framework managers can make final decisions about which dev dependencies to remove.
-
Blocker: 📌 Add php-tuf/composer-stager to core dependencies and governance — for experimental Automatic Updates & Project Browser modules Needs work (This is blocked on the core MR above.)
-
Blocker: 📌 Package manager/ Automatic Updates should disallow composer patches by default Active
-
Non-alpha-blocker: https://github.com/php-tuf/composer-stager/issues/300 (This can be treated as a docs gate stable blocker.)
-
Non-alpha blocker: The current performance issues with the server-side implementation can be treated as beta- or stable-blockers, but do not need to block an alpha commit.
-
The issue is a priority across the board as a blocker for basically every other important thing we're doing (including Starshot), so we want to try to focus resources on it. To that end:
-
Lauri will speak to Dries about adding a section to the Driesnote about this.
-
Separate issue needed: Contact the DA about issue credit bonuses for this and its related blocking issues (probably as well as followups through AU-stable.)
-
We could accelerate work with faster review cycles, so coordinating our work such that the technical leads have time to respond when the FM and RMs provide reviews would also help us make faster progress.
Hopefully I didn't miss or misrepresent anything. 🤞 Thanks everyone for your dedication on this issue!
- jQuery and jQuery UI should be on the list for sure; we are very dependent on their release cycles and we have contacts with their maintainers.
- We also will need detailed sections about the various Autoupdates PHP components.
I'll have a think about others that should be on the list too for release management and security stuff.
Thanks for filing this; it should make the page a lot more maintainable to focus only on the key dependencies.
Two other things:
- We should establish a best practice for which dependencies have their versions listed. I believe the list still has dependencies that were removed in D10. I think we should:
- Add a note on the page if the dependency is to be removed in the next major version.
- Remove listings when the last version to require them goes EOL.
- Maybe we should instead have a way of tracking dependency evaluation issues (new issue tag?), and those issues should be expanded to include the contact details etc. That way we could get the info at need for less-important dependencies, rather than having to maintain the information in a second place. There could be a link to these issues at the top or bottom of the doc.
quietone → credited xjm → .
pameeela → credited xjm → .
pameeela → credited xjm → .
hestenet → credited xjm → .
hestenet → credited xjm → .
smustgrave → credited xjm → .
I added a comment to 📌 Update to jquery UI 1.14.0 beta Downport tentatively signing off on backporting this to 1.14 as well. (I commented there because there are some changes to the core vendor tooling script that look like they might've needed to be done by hand, but we could also mark that issue to fixed and open a 10.4 MR with the full update on this issue.)
Discussed with @mgol, @catch, and @nod_. Based on the discussion, I think we should indeed also backport this to 10.4. We'll need to modify the release note a little to mention that there are a handful of small BC breaks in the upgrade guide that may require updates to custom code, but overall it's fairly non-disruptive for us and it fits with the objectives for maintenance minors of keeping up with secure dependency versions.
I added a similar link to https://www.drupal.org/project/drupal/releases/11.0.0 → (linking the new Drupal 11 landing page).
I moved outstanding active issues to the equivalent Drupal 12 meta.