@dww 📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed allows you to configure a cron job to run automatic updates, this would (theoretically at least) allow someone to configure automatic updates to only ever do anything via that cron job with a non-functioning UI (because the UI doesn't have permission to write to the file system). But yeah the capability to have the UI work with non-web-writable directories was never added and is what this issue is open for.
The UIs themselves aren't in core, but to me it makes sense to have the queue implementation in package_manager so that both automatic updates and project browser can be updated to work with it.
when it's meant to be a drop-replacement for a persistent cache, like we do in kernel tests for example.
In kernel tests we're using the test-only memory cache we added before any of this that does serialize()/unserialize(), this is because otherwise the behaviour of setting/getting an item from the cache is different to if it's in the database or apcu etc. which would not be good for kernel tests.
For the actual memory cache we don't want to serialize/unserialize, but this means it isn't a 1-1 drop in replacement for persistent caches - obviously you can swap them around if you realise something does/doesn't need persistent caching but it does behave differently, which is why we added MemoryCacheInterface.
📌 [PP-1] Clean up MemoryCache and MemoryBackend naming issues Postponed is open to clean all of that up.
cache.backend.memory should not be using MemoryBackend, that's only reference by cache.backend.memory - we could probably clean that up here - make cache.backend.memory.memory an alias and deprecate it, so we're not using the testing code in runtime.
Discussed this briefly with @lauriii, he asked why we're adding a feature flag for this at all - it only affects existing sites when they add a field in the field UI, not the actual runtime of the site - so we should be fine to just change the behaviour with no bc layer.
Thinking about it more this makes sense to me too, I tried to trace back where this was discussed and I see Damien's comment asking whether it was really necessary, but otherwise we only discussed how to do it, not whether to do it or not.
Given that would allow us to remove a fair bit of code + and upgrade path + the follow-up issue, I think we should just do that here. It will mean updating some tests but that would need to happen anyway to remove the feature module.
Reverted for now - let's add the deprecated module documentation and update the test.
Yeah agreed we shouldn't block this on a test, it's a case that clearly falls under 'may not be needed' on https://www.drupal.org/about/core/policies/core-change-policies/core-gat... →
Committed/pushed to 11.x, thanks!
Yeah removing the counts for now sounds fine - the total across all projects is listed on the organisation page anyway.
I think we could probably use route aliasing on the deprecated comment route as well, per https://www.drupal.org/node/3317784 → ?
This looks good. I was going to ask about where we're going to remove it from all the tests it's added to, but then I saw 📌 Update layout builder tests and set new config Active , we might want to split that issue into two, but can discuss on that issue.
Committed/pushed to 11.x, thanks!
We'll need to mark the module obsolete in Drupal 12 + add an update in the module to uninstall itself (same as other previously obsolete modules).
I think it might be good to split the test changes here out to a new issue since that could also happen in 11.x, so that the 12.x MR is just changing to obsolete?
I think this is in a good enough state to mark fixed now - if we need to make small changes to the wording we can figure that out in slack without an issue. If something big happens with PHP release cycles we'll need a new issue anyway.
This looks good, nice that we can get rid of @legacy-covers again pretty easily.
Committed/pushed to 11.x, thanks!
An alternative idea I had was to add the user cache context, so that cache context manager can filter out automatically based on optimization rules, but I need to figure out how much faster that actually is.
This seems definitely worth a try. It's a case where we get zero benefit from the cache not being per user, so there shouldn't be any downside, it's just a question of how much improvement there is.
Updated page at https://www.drupal.org/about/core/policies/core-change-policies/set-plat... → looks good to me. Only nit is we should change the 'statement' header to something else or remove it.
Haven't done a full review yet but +1 to copying the assets across. Does the MR so far handle deleting the assets and copying them again so they can be updated?
What are the concrete things that need to be done in this issue? As @quietone says, all four child issues are closed (fixed) and that is the only task in 'remaining tasks'.
@andypost suggested closing this issue eight years ago, in 2017.
You yourself, also in 2017 in #2323895-45: [Meta] Document format/content of various YML files → suggested that #2823463: Devise an extensible way to document extensible data structures → would cover anything remaining here. Do we need two issues open for one proposal?
So I can't think of a more appropriate status than "Postponed (maintainer needs more info)" because no-one has had any suggestions for additional things to do in this issue for eight years, and several things were done prior to that which covered the scope of the initial report.
The screenshot attached to the issue summary is for https://www.drupal.org/third-and-grove#org-page-issue-credit → , it shows 9 Drupal core core issue credits for the last year. I would except that number to be somewhere between 500 and 1,000.
When you click through to https://www.drupal.org/node/2373279/issue-credits → the last five issues on that page are Drupal core, which matches the 5 issues shown on the summary. This suggests that the summary is generated only from the first page of the pager.
e.g. page 8 of that view shows Drupal core credits from April https://www.drupal.org/node/2373279/issue-credits?page=8 →
I would expect the high level credit summary to list all credits for each project listed (and probably every project too), pretty sure this is how it used to work prior to the new credit system.
This looks good to me - encouraging that it's a bugfix for the current logic rather than reverting closer to how it was previously.
Didn't manually test it but looks like @msandoval did.
Committed/pushed to 2.0.x, thanks!
Committed/pushed to 11.x, thanks!
Yep let's do it - we can always revert if we think we've made them generally more reliable, or use #[Retry] when it's available.
I haven't been involved with this module since 2008 (!) - I made it solely to enable the same feature to be removed from Drupal core. It looks like there have been no commits from anyone since 2020 so to me it makes sense to accept the offer from @sagesolutions - I've granted you all permissions except for 'administer maintainers'.
Moving back to the project queue and 'fixed' but if that's the wrong place/status feel free to change.
Couple of minor points on the MR. Overall it's looking great.
The main overall question I have is how we'll eventually phase out the AJAX logic in here and more clearly delineate what's done for AJAX and what's done for HTMX and what's done for both, but I didn't have any good ideas when reviewing.
Committed/pushed to 11.x and cherry-pick (with a small amount of manual merge conflict resolution) to 11.2.x, thanks!
Committed/pushed to 11.x, thanks!
There is also the situation where the site template developer/creator needs to customize the theme, in which case a fork or runtime dependency is needed.
I think it's reasonable to expect site template authors to keep up with changes in Mercury - otherwise the theme will be permanently out of date (e.g. an old forked version) for new sites too.
If it is a runtime dependency then the user can't quite use it as a starterkit, since that can't flatten the dependency automatically. What do you think of that situation?
If the starterkit runs on both the subtheme and mercury, would that cover it? You'd need to run it twice and maybe update the dependency, but it doesn't seem that much different to running it once.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks! Looking forwards to working with both of you on asset issues!
I think a follow-up is good, but yeah let's open it - 12.x will be open soon.
Still have some vague undefined reservations here, but hopefully those are unfounded, certainly nothing concrete any more - I think the class_exists() check resolves most of them anyway.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
This removes the rsync and composer config from the schema without removing the config keys themselves - this is to support the hook_requirements() pointing to settings.php.
However it's not clear to me how someone is supposed to actually remove those config keys from a site?
We might need to add an upgrade path and skip the hook_requirements() - there should be one for when composer/rsync can't be found anyway.
Or if we otherwise want to keep that, how do people remove this from config?
Package manager isn't beta yet so manual steps are OK but a lot of sites have it installed via Drupal CMS now and we shouldn't cause them to have data integrity issues without an obvious way out.
For 3, maybe we start merging via GitLab?
We historically haven't been able to do this because it wasn't possible to customise the author on squashed commit messages, and probably other things too in the past, but iirc that's the last one, I think that still might be the case but haven't checked in the past month.
It would be _great_ to be able to use merge trains, but I feel like this issue is one of still several steps before we're able to do that.
Add another heading to the default issue body template called “Draft commit message”.
This feels like it would mean having to maintain both the issue credits themselves and the commit message separately, in different interfaces, and that they would get out of sync all the time. The commit message is also currently generated from the title, but that would also now be maintained in a separate place (or just disconnected entirely).
I'm not sure how a (more) collective solution could work technically. Currently both committers and subsystem maintainers can assign issue credit (hopefully this is the case with the new system) - when this is already done on a complex issue it definitely helps, and is a collective process.
However the commit message itself is automatically generated from the contribution credit and people's own accounts/metadata, so any manual tweaks like changing 'fix' to 'feat' or whatever have to be one by the actual person making the commit at commit time (not even people with commit access in general). With the new system I have to exclude myself from commit messages if the only thing I'm doing on an issue is committing it, then manually add the contribution credit for the commit - either doing it in the right order or manually editing the commit message to remove myself.
If we get into things like making sure people's email addresses are correct (I managed to end up with three in core commit logs - the no-reply d.o one I was trying to use, my personal email when I forgot to set no-reply on a new machine for a while, and a second no-reply one in a different format that I'm still not sure where it came from), then that is a lot of additional admin work that only the person being credited themselves actually knows about - and they may not have been on that issue for three years.
Issue credit is already the most time consuming part of the technical process of committing an issue (excluding review time, although one line commit typo fixes with 15 people on an issue, not even excluding that). So I'm very resistant to anything that increases that time, even by a little bit.
With #38 do we know whether gitlab's integration works without an email address or not? If not, I'd be fine with just a comma separated list, but it would be shame to change things yet again if it turns out gitlab can handle the format without email addresses.
I didn't realise that both the node and comment user picture rendering is behind a setting - although I think themes have to individually implement that setting and the UI for it?. That might mean ✨ [META] Expose Title and other base fields in Manage Display Active covers the rest here. Back to needs more info for now.
Agreed that trying to multiple render the users is no longer going to be useful with render caching.
This is still a problem.
Briefly mentioned this to @lauriii and @phenaproxima in slack, but it was only a couple of hours ago so the write up may already have been in progress by that time.
The issue summary from the other issue is for me a serious problem:
When you install a theme from a site template, we won't support updates, and the theme should not be update-able (via Composer or AU).
"We won't support updates" was not discussed further either in that issue or this one but seems pretty critical given Drupal CMS is supposed to support automatic updates.
I don't think it's reasonable to put the entire burden of updating a theme for major releases on the people who install site templates. While it's not reasonable to expect them to understand the complexities of sub-theming either, having to prepae a custom theme for major version compatibility changes by themselves could mean they're never able to update their sites.
For a similar-ish situation, see this thread in #support today from someone who installed Drupal 11 to try to move away from Wordpress, then wanted to use a commercial theme that only has Drupal 9 compatibility (because it has the design they want). They obviously have no idea how much work it would be to update that theme for Drupal 11 compatibility, and neither do I because it's not open source. Now multiply that by literally everyone who installs a site template where the theme is starter-kitted to a custom one - except in that case they also wouldn't even know in advance that this works lies ahead in 1-3 years.
Given that Canvas will eventually allow a fair bit of customisation without either a subtheme or a custom theme, I think it would be better to look ahead to that situation - make customisation a bit harder out of the box now, but allow updates to be easier.
Something like:
1. Themes shipped with site templates should be composer updatable.
2. If site owners want to customise a theme, provide instructions on how to create a theme using the starterkit command - but don't do this for them, it should be an explicit step taken by the site owner.
3. All the sites that don't customise their theme will eventually be able to do so via Canvas more and more as features are added.
The lookup keys were due to #3497954-16: Excessive memory use when building dynamic library definitions → and #17, I didn't realise they were still in the newer versions of the MR so was temporarily confused by #32. Those can indeed just be removed as lookup keys now that it's using state.
We should try to get this back in once 📌 Remove the ability to configure the path to Composer Active lands.
I think we might want to make this even more special cased and do a ::moduleExists('automated_cron') first - nothing else should run cron in an end of request task.
There are other things that can happen end of request too, but:
- they should be a lot quicker than a cron run
- I can't think of any way to generically handle those, except for breaking end of request processing for everyone including outside tests.
I've done that quickly, but also I think we should at some point move that to a README.md or handbook page on Drupal.org and out of the project page so it's easier to update in the future - can be a follow-up though because we'll need to decide where.
Moving to RTBC.
I think that could be because you're building the message in a variable, then putting that entire variable in the trigger_error(), and maybe the phpcs rule just doesn't pick that up at all? If you change the message does it notice?
I thought I'd got around this in the moved classes classloader, but it looks like the way I did that was a phpcs:ignore too: https://git.drupalcode.org/project/drupal/-/commit/9244acff98497d4b7961b...
@maya maier I have access to create releases and I can make a release.
I think we should probably add 🐛 H5P settings not always available in iframe Active to the beta1 blockers if 📌 Use div embed regardless of H5P plugin Active has been descoped to beta2 because they affect the same code.
I think the main thing is some general testing once the individual issues are all merged to make sure the combination of two issues hasn't caused an unexpected regression.
I think that's 📌 Slowly, very slowly start OOPifying the render system Needs review .
There's also 📌 Support dynamic forms using HTMX Active and several other HTMX issues which will gradually replace #ajax.
@anaconda777 are you doing that with the H5P field formatter? This change would only affect that and nothing else.
@amyesther loading a node in a middleware isn't really supported or encouraged, see 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review and 🐛 Shield middleware invokes hooks before modules are loaded, corrupting module_implements cache Needs work for some discussion. There are other request events you could hook into if you really need the logic to run early in every request (just not so early that modules aren't loaded yet).
That works for me. It will be a bit annoying if the same closure is used in multiple different hook methods, but we can fix that in 12.x with property hooks anyway.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
I think the helper is worth it - we have at least one place to use it in core and several in contrib. If we ever decide it isn't worth it, we could deprecate the new helper.
Committed/pushed to 11.x, thanks!
Given this is for removal in 13.0.0 not going to worry about a backport to 10.6
I think this is ready to go. We can make very minor tweaks to the wording once the policy is in place if we need to, but I don't really think we're discussing the policy itself any more.
We have 📌 Allow omitting doxygen when type and variable name is enough Active which would mean no extraneous docs for the method in this case, but it's not closed yet.
Yeah #13 looks really good, a new issue makes sense, maybe a meta for using property hooks with service closures or something like that?
Opened 📌 Add performance test for saving a node Active for the node save idea.
One comment on the MR.
I still think that adding all the formats to existing sites would be confusing (to be honest I also still think this issue could just add the one date format instead of three but I am neither a UX nor product manager).
If we need to add exactly one new date format, checking for existing formats with the same name, we could do that - but IMO that should only happen in the announcements feed module then, so that it only gets added for sites with that module enabled. This would not be the same update as the one that was removed after #33, maybe some of the logic can be re-used though.
There's also the 'breaking change' which can be represented by either a BREAKING CHANGE: note or an exclamation mark like feat!. It looks to me like this is supposed to be used for actual breaking changes, not deprecations, so I don't think we'd use it much in core - maybe for deprecation removals but those are usually the only point of those commits.
Yeah I think that's fine and could always be changed later. The MR as it stands should significantly improve PHP memory usage when there's a lot of webforms, including when none of the CSS or JS features are used (whether per-webform or global)
Drupal core doesn't have access to legal counsel and never has done. I'm surprised I even had to type that sentence as it's so far removed from the reality of core development.
The GPL is explicitly provided with no warranty or liability.
As @penyaskito mentions if someone requests not to be credited on an issue we remove them.
For me 'by' and 'co-authored-by' both imply authorship, just the latter sounds artificially a bit grander.
I personally do not care if we use 'by' or 'co-authored-by' but I do care if improvements to the commit message format and gitlab issues get delayed by bike shedding over entirely hypothetical situations.
That is odd but I think we should go ahead here and open a new issue just to double check it's unrelated.
Everything else looks straightforwards so moving to RTBC.
It'd be good to open an issue to use this in \Drupal\media\Plugin\Filter\MediaEmbed::settingsForm() and anywhere else that makes sense.
Thanks for opening the follow-up.
Committed/pushed to 11.x, thanks!
The ckeditor team has confirmed that the vulnerability was introduced after 44.0.0 was tagged, so we don't need to do anything for 11.1 and 10.4.
Makes sense - cherry-picked to 10.6.x, thanks!
Updating the issue summary to reflect that we converged on Co-authored-by
@frob take a look at the H5P field formatter and the difference between the div and iframe embed logic in there.
The comment was on the wrong issue. The three commits that were reverted, were:
📌
Allow composer location to be configured via the UI
Active
✨
Dynamically figure out the actual path to Composer's binary, and make it read-only
Active
✨ The vendor hardening plugin should support changing the permissions of certain paths Active was also reverted, but later re-committed again with some changes.
Moving this one to fixed, thanks for checking!
Moving 📌 Local task name expectation in getFeaturedPageActions is fragile Active to nice to have, per the discussion on the issue, see #3511612-12: Local task name expectation in getFeaturedPageActions is fragile for determining Top Bar featured actions → and above.
I think it would be a good idea at this point to merge this issue back into 📌 New Toolbar Roadmap: Path to Beta & Stable Active so that all the must/should have etc. issues for the navigation module are tracked in one place - it's only one module that provides two bars, we can have subsections on the main issue, but it's hard to follow with it split like this - and the other issue has some top bar issues already.
This looks fine, let's get it in so we can get closer to a green test run.
Committed/pushed to 11.x, thanks!
Now that we use the render cache for a lot more things, I don't think locking at that level would be a good idea - the lock acquire overhead is non zero, most pages have several render cacheable items on them, and the cardinality of most cached items is quite high.
There are individual cases like header or footer menus that appear on every page, it might be possible to lock these, but this could be implemented at the level of individual block plugins (e.g. we could implement a lock in system menu block rendering).
📌 Use revolt to prewarm caches during lock waits Active might entail adding locking to more low level caches though - theme registry, library discovery etc. that can be pre-warmed.
No, it's database schema, and less disruptive than another schema change would be.
When I look in git blame, I look for the issue number and the names of who worked on an issue - it gives me quick context about who worked on it which is often useful. I will often still need to copy and paste the issue nid and then take a look to see if there's the discussion of the line I'm interested in, but sometimes seeing whether I worked on the line in question or not is enough too.
We can probably only remove one or two, the problem is knowing which ones are safe to remove.
This still needs to be done I think but it's not easy.
We'd need to check all the queries that get run on these tables, then check which of the existing indexes they use if any. There might be another issue around this.
Committed/pushed to 11.x and cherry-picked to 10.6.x, thanks!
Committed/pushed to 11.x, cherry-picked to 11.2.x and 10.6.x thanks!
Thanks for figuring out this was a duplicate. Did my best with issue credit and closing out.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
Unpostponing this.
I think we should consider removing the hook_requirements() from here altogether, and sorting that out in 📌 Properly handle that PHP bcrypt passwords are truncated to 72 bytes Active where we have multiple options that might not require it (at least on all sites, we still might end up with something similar on some sites).
Yeah I think #5 might be enough - this ought to be testable locally since it affects composer's local behaviour rather than packagist (I think?).
Will issue credit be properly picked up, if the format ends up changing?
Issue credit isn't based on the commit log, the canonical source is the Drupal.org issue credit records, these are then used in the UI to generate a commit message. So there's no actual effect on commit credit except how this influences the d.o UI.
The thing that could be broken by more than one commit message format change is other things based on commit log parsing like codeswarm.
I think if PHP changed their release timing significantly we'd need to change the policy again. e.g. if 8.5 stable was in March, then that's very little time for it to filter through before a June Drupal 12 release, so in that sort of situation we might stick to PHP 8.4.
How about this?
"The minimum PHP requirement is set to the highest stable PHP version available at the time beta1 of the next major version is released, assuming support has already been added in a Drupal minor release."
Because it's really adding PHP 8.5 support to 11.3 that allows us to require it in 12.0 - gives six months for contrib to update for it against a green core test suite and stable PHP containers etc.
The other thing the current policy doesn't really account for is a new major PHP release, e.g. if PHP released PHP 8.5 and 9.0 at the same time, or only 9.0, then we couldn't automatically adopt 9.0, we'd need to discuss it.
I think a core module for argon2 is good idea, but also think we should probably pre-hash passwords when it's not installed.
The one difficulty is that once we've pre-hashed a longer password, we'll need to account for that even if a site enables password_argon2 - so it would need to try argon2 first, then fallback to pre-hash + bcrypt if that fails (and rehash that as argon2). But that's compexity for us, not for site owners who shouldn't need to understand the difference between bcrypt and argon2.
Another option would be to enforce argon2 for new installs, and have a bc module for bcrypt in case any sites are on hosts that don't support argon2, which we could eventually deprecate.
One possible way to handle this:
1. Continue to point packagist to contrib gin for drupal/gin
2. Contrib gin releases a new branch for >= 11.3 that has only a README in it - this way the core theme will be used on all 11.3 sites.
3. (possibly) core should specify provides or replaces for drupal/gin in 11.3 so that composer assumes it's already there.
I think this would mean that sites updating to 11.3 would be automatically switched over to the core version of the theme.
It would make it very difficult for sites to install contrib gin on 11.3 but that might be a 'feature' and/or we could document how to do it.
If you look at the MR output for kernel test jobs, you can see how long the test takes to run.
26.121s Drupal\KernelTests\Components\ComponentRenderTest 14 passed
The longest Kernel tests are over 3 minutes so there's no benefit to consolidating the methods here and the MR should be fine.
With the change record, is password.options really required? It looks like it defaults to empty array anyway and could be omitted.
Also should some documentation be going into default.services.yml? The issue summary mentions this but there's no changes in the MR.