The idea was to allow recipes to install node module with a text_long body field, but recipes can already do that since they skip installing module config. So yeah this doesn't do anything, we need 📌 Remove node_add_body_field from NodeTypeForm Active instead. Closing as duplicate of the other two isseus.
Tried clicking the button in a different way etc. but did not really get anywhere.
A simpler (except for annoying nightwatch test coverage) attempt to prevent this being necessary happening on 📌 Remove node_add_body_field from NodeTypeForm Active
The new test coverage looks good, marked threads on the MR resolved because I agree with the answers given.
Tagging beta target because this should significantly help mitigate the increased memory usage due to the procedural hook bc layer.
MR is green again now. Updated the issue summary with a summary of the implementation.
It's my attempt at garbage collection breaking. Pushed a commit which fixes at least one test.
Pushed a commit to implement #4.
Pipeline is green.
There is one remaining thing, which is we need a new FileCacheGarbageCollectionInterface, implement it in the new class, call the ::garbageCollection() method in system_cron() if the service implements the interface. That will complete the expires logic added so the database table doesn't have stale and un-removable cache entries in it forever.
Already deleteable via the API, just not via the UI.
Discussed this briefly with @Gábor Hojtsy in slack and he approved this change from a product manager perspective. It changes what happens with the form, but since the field is already deletable it doesn't affect other expectations outside the UI.
Drupal 11.1.0-rc1 is next week and I think the core patch could still theoretically land, although it's not looking as likely as it did a week ago. I don't know how this works with Drupal CMS timings and whether this could be reverted in a week if it needs to go in this week etc.
This should probably get official product manager sign-off too, although that may already have happened on one of the 2-3 related issues.
I checked the two modules with 11.x releases.
First views_rss: The body field is referenced by tests/modules/views_rss_test_config/config/install/views.view.test_views_rss_feed.yml. Depending on what the test does, this may or may not require changes but either way it would only affect the test module, no runtime code.
I also checked views_kanban, the body field is in
Demo/views_kanban_demo/config/install/field.field.node.task.body.yml
so not quite a test module, but a demo module.
When max_input_vars is exceeded an E_WARNING is triggered according to https://www.php.net/manual/en/info.configuration.php#ini.max-input-vars - could we special-case this in the error handler and then do something with it?
I think #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole → is a duplicate of 🌱 [policy] Standardize how we implement in-memory caches Needs work .
Re #99 we could probably add RequestAwareMemoryCache or something like that, it would only be necessary for some caches, not all.
For caches that are unbounded but also don't need request context, we need #1199866: Add an in-memory LRU cache → .
@hestenet I'm not sure how these can be reliably limited to site owners, if that's the intention, they should be descoped.
What admin paths the user has navigated to
Which forms have been started/completed
Indicators of confusion/difficulty in navigation, such as:
Multiple rage clicks
Multiple clicks and back button uses
Etc...
Session replay capability - to be able to view the actual use of a page over a period of time (effectively, distributed, anonymized user testing).
On a more basic level, because there's no drupal_cms module or install profile, it's not clear to me that usage of drupal_cms will be reported to update status/d.o at all - that might be a more urgent thing to figure out.
Test needed to be updated for a different error message, just an expectation change. Moving back to RTBC since the fix is trivial.
Tagging for release highlights and a change record. Would be great if we could summarize the discussion above in the CR since I think it will help people understand the new feature.
RTBC for me. I wondered about removing the rest of the phpdoc (and maybe even the constructor itself) too but not sure there's much benefit to that, you either stop at the class summary or you don't.
So if this hasn't already been considered, I would strongly support adding this option to vocabularies/terms, in addition to nodes.
The current MR almost supports this, the main (only?) thing missing is the route provider logic which is node-specific. For taxonomy terms that might be further complicated because the taxonomy/term/n page is usually overridden by Views, so maybe a bit extra to make it work for taxonomy terms but otherwise all the bits are there. I think we should have a follow-up for applying this to taxonomy terms, the use-case makes sense.
Could also be good to replace media's standalone_url: false setting with this too? But probably also in its own issue because that would involve bc/deprecation for the existing setting.
@smustgrave @alexpott is concerned this will cause issues with modules that ship config expecting the body field to be available - book and blog are two, there are several others but per #26 after looking through a sample I was unable to find one with a stable release. For me that should be OK with a change record. It won't affect existing sites with any of those modules installed, because they already have whatever config they installed with, it would only affect installing them from fresh on a site (and even then existing sites will have text_with_summary anyway, so it would be on new sites installed on versions after this change).
I personally think we should go ahead here once https://git.drupalcode.org/project/drupal/-/merge_requests/10019#note_40... is fixed but it might need more discussion - tagging for Needs Release Manager Review and I'll bring it up with other committers.
I'm getting errors from commit-code.sh trying to commit this locally.
yarn check -s says:
error "acorn" is wrong version: expected "8.12.1", got "8.11.3"
warning Resolution field "ejs@3.1.10" is incompatible with requested version "nightwatch#ejs@3.1.8"
warning Resolution field "nightwatch#semver@7.5.4" is incompatible with requested version "nightwatch#semver@7.3.5"
warning "postcss-preset-env#autoprefixer#caniuse-lite@^1.0.30001599" could be deduped from "1.0.30001664" to "caniuse-lite@1.0.30001664"
error "espree#acorn" not installed
error "espree#acorn-jsx" not installed
error Found 3 errors.
This might be just an issue with my local so leaving RTBC.
can't do that alas - attributes are a PHPUnit 10+ thing, and D10 is testing with PHPUnit 9.
But if we're doing the discovery, does PHPUnit 9 care? (apologies if this is a silly question). Is the problem that the attribute class wouldn't exist?
Committed/pushed to 11.x and 11.1.x, thanks!
We should backport this to 10.5.x/10.4.x so moving there.
Committed/pushed to 10.5.x and cherry-picked to 10.4.x and 10.3.x, thanks!
Re #6 I think this is no longer an issue in 10.x/11.x due to:
https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/HttpFo... e.g. a BadRequestException is already thrown by Symfony if the value retrieved in ::get() isn't a string.
Thanks added a note to the issue summary.
Which issue is this postponed on? I can't see anything listed in the issue summary.
Tagging as beta target because it's exacerbated by the latest Symfony release.
That looks good - maybe the below is a bit more explicit?
This class is included in earlier Drupal versions to prevent phpstan errors for modules implementing object oriented hooks using the #Hook and #LegacyHook attributes.
When I've looked at this problem for
#2719315: Try to install system module like any other module →
and similar issues I've repeatedly got stuck on wondering what really needs to be added from the BareHtmlPageRenderer vs. what could possibly only be added by system_page_attachments() - but just factoring out the entire method as a start makes sense and avoids getting stuck on what could potentially live where.
Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!
Potentially this will unblock #2719315: Try to install system module like any other module → or at least get us part of the way there.
This could use an issue summary update and a change record to indicate that attributes will be supported. We'll need to decide if we backport this to 10.x so that contrib tests using group attributes, I think that has to be a yes if we want people to use it.
Also should we have a follow-up to try to use phpunit's discovery or did you end up deciding that's too far off?
I think this is probably fine.
There is a use-case for password reset links where people create a password, don't try to remember or it use a password manager for it, then use reset links each time. drush uli is a similar use-case during local development too.
However, because our password reset links also log you in, making the fields required won't break this - e.g. you can click the link, ignore the form, same as you previously did.
Approach in the latest MR looks good to me too.
@berdir that sounds potentially about right for the reduction - I think it would affect any test that has two @group annotations (other than @group #slow which was already made unique).
There's #2624926: Refactor run-tests.sh for Console component. → open to refactor run-tests.sh. I also think there's some functionality we can drop, although we'll need to be careful with the contrib gitlab templates which use different features to core.
Yep agreed with #16.
@bkosborne yes that's the expected behaviour with the MR.
There are other things we could do, like make a subrequest with a sanitized path or similar, but to me it feels both more complex and less robust.
I think we need to decide whether we want to remove support for procedural hooks in Drupal 12 or 13. For me, even if it's automated, it's probably going to be too much to remove support in Drupal 12 - there are a lot of sites with a lot of old custom modules.
Also even though I know the reasons why, it feels a bit hard to say you can't have a .module file but you have to have a .install file - giving ourselves a bit more time might mean we can figure out alternatives for hook_install() et al by then.
We've got two main problems keeping the bc layer in:
1. The performance cost of legacy hook discovery. If 📌 Add a feature flag for the procedural hooks bc layer Active works we could make that opt-out in Drupal 11, opt-in in Drupal 12, gone in Drupal 13.
2. Hook ordering and etc. if we can come up with an all-OOP hook ordering system, I think it would be good and fine to deprecate hook_module_implements_alter() for removal in Drupal 12 - if you have procedural hooks in Drupal 12, you don't get to order them and it's a bug report/task against the module to convert if it breaks something.
If the above is roughly OK, then deprecating .module files in their entirety, for removal in Drupal 13, starts to sound pretty good then - we give people a lot of warning and then do a clean sweep, and they've got until the end of Drupal 12 support to get ready.
I had assumed we'd trigger E_USER_DEPRECATED in hook discovery, once per module when we find a procedural hook, should be cheaper?
With this change, DefaultPluginManager test only shows up in job 2:
Thanks this looks good, nice catch on the moved toolbar hook implementation.
Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!
Ahh #43 is a good point, however I think we're OK here because those implementations will still be discovered by the OOP attribute discovery (just not invoked when hooks are invoked), whereas the problem we have with experience_builder is the procedural implementation not being discovered at all until the module is installed. There's no way to tell which module foo_bar_foo_bar_foo_bar() is implemented on behalf of, until foo, foo_bar or foo_bar_foo module is installed, but the attribute is explicit.
Committed/pushed to 11.x and cherry-picked back through all branches to 10.3.x, thanks!
Not sure what the fix is yet, maybe we need to hash the regex as part of the cache key?
The concept of implementing a hook on behalf of another module doesn't exist with OOP hooks (you can check moduleExists() inside a hook implementation of course still), so we only need to cover this with procedural hooks, so I tried implementing it only for them - seems simple enough but see what the pipeline says. The advantage if we do it this way is we only take the hit on the bc layer and only when the module list changes, so regular drush crs still get the benefit as do converted modules.
Allows users to stage content or preview a full site by using multiple workspaces on a single site.
Change to maybe this?
Provides an API for staging and previewing content in a full site context.
catch → changed the visibility of the branch 3478621-add-a-filecache-backed to active.
No it is that, I just failed to push the correct branch.
OK it's not that, might be to do with config_inspector but I can't see any direct hook invocations, which leaves 'something weird' or drush.
I think that is specific to this MR and that job. The 11.x branch tip here was from before OOP hooks so it was getting very confused. I've pushed a rebase of the 11.x branch here and will re-run the job.
It will make it harder to search/grep for implementations
Yes as someone who constantly greps looking for code and who doesn't use an IDE, it would be nice to retain this ability.
The fact that it's easy to set the moderation state of a node in-code, just by calling ->set('moderation_state', '...') really helps enable these more complex workflows.
Hopefully this will stay exactly the same.
The current idea is that there is a primary entity for a content moderation workspace, e.g. when you edit or create a node, we create a workspace for that entity and then edits to other entities within that workspace are included, but the workflow status is still tracked on the node itself. When the workflow state moves to published, this publishes the workspace and the other downstream changes (media, path aliases, menu links, paragraphs etc.) are published alongside it. This way the current UX and data model would be preserved as closely as possible, e.g. we'd be adding extra data but not necessarily moving or removing what content_moderation already stores.
Good point - pushed a commit for this, since it's a trivial change, moving back to RTBC.
Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!
From the issue summary, this could be proposing one of two things which are very different, and it's not clear to me which one it is:
1. Telemetry of Drupal CMS installations and admins themselves - e.g. which recipes get installed by project browser or similar things that would be an extension to what update status already does. Or possibly exposing some of the existing update status data that is not available on Drupal.org (like which core modules are installed) via a page on d.o
2. Telemetry of the visitors to Drupal CMS sites which to me has extremely grave privacy/GDPR implications since the site owner would need to somehow indicate they are sharing their own users' information with a third party (the Drupal Association).
I think I saw some kind of draft somewhere about this, and that also was not clear whether it was talking about #1 or #2 but seemed to suggest it could be both.
Obviously firefox or Mac OS telemetry is only able to do #1 since those are assumed to be single-user installations, this is not the case for a website.
The original issue title was overly optimistic, maybe we can come up with a generic filecache-backed attribute parser but I think we should do a couple of one-off issues like this before that to understand the requirements better.
Feels obvious now that you've pointed it out - the file cache depends on the filename and can't use arbitrary cids.
However, when I went to remove it, I realised we can probably add it to the namespace instead - seems like this should work but let's see what the pipeline says.
Two very common random test failures but otherwise green. Moving to RTBC.
Needs another rebase.
Two minor nits on the MR but otherwise RTBC for me. Would be good to get this in before the 10.4 beta, although I also think it could go in during the beta if necessary.
@iselectweb I've opened 📌 Use the assets stream wrapper for translated javascript files Active . The actual code change should be pretty minimal, so once there's an MR, one way to demonstrate the problem here would be to try it out and see if it helps or not. I think that issue makes sense in its own right even if we don't have conclusive information about the problem you're experiencing - the generated javascript translation files are pretty similar to asset aggregates. Can't guarantee if/when I'd be able to work on that issue but please follow - it's also possible someone else will pick it up.
This issue is very tricky, both postgres not supporting joins on int vs. string columns and the primary key/not null handling, however it looks like all alternatives have been exhausted so let's go ahead.
Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!
A review for a MR, or a set or MRs changes that workflow; that means the policy must be changed to describe what exactly must be done in those cases because there is no way to suggest changes that should be applied to project files
The issue summary is not suggesting that someone should manually review a core MR for code quality - in the case of an MR that has been committed, this has already happened by other core contributors and committers anyway.
What is suggeste is that people should be able to post an issue asking for the git vetted role, pointing to a history of meaningful contribution to core (or high usage contrib modules) via MRs and issues, in lieu of a module review.
If we made a comparison to a job interview, then the module review is like a technical interview, and the list of issues/MRS or link to MAINTAINERS.txt is more like a resumé or CV.
The most review I would expect the process to involve is that the person actually did contribute to the issues they said they did and hasn't just linked to some random issues, clicking some links and ctrl-F to see some comments/commits happened and are somewhat substantive would be enough.
People appointed as Drupal core maintainers (or sub-maintainers) already do not follow what reported in Offering to become a project owner, maintainer, or co-maintainer,
There is a clearly documented process for becoming a core subsystem maintainer here: https://www.drupal.org/community/contributor-guide/role/drupal-core-subs... →
so I do not see why they should follow what reported in Apply for permission to opt into security advisory coverage, except in the case Apply for permission to opt into security advisory coverage makes an exception for Drupal core maintainers. (
Permission to opt into security advisory coverage only has one process, which is module review, unless you bypass that process. We are trying to add that process (or exception if you want to call it that) here. Core subsystem maintainer is a different role to module co-maintainer with its own set of responsibilities and expectations. The git vetted role is... the git vetted role.
However I would also reiterate that core committers/subsystem maintainers are an example of the kind of people who are not well served by the current policy, they are not the only people that the policy change is intended to help. For example I was contributing to core for years over hundreds of issues before I became a subsystem maintainer (or got what was then cvs access), and there are similar people who do that now who aren't in MAINTAINERS.txt too - however it would have been easy for me to link to a few issues like http://drupal.org/node/6162 that I'd been working on.
The question is then: Why should people who are going to be appointed Drupal core maintainers be considered at the same level of people who still do not have the vetted role, and who probably wrote a module for their first time?
That is the current status quo, and what we are trying to change here.
smustgrave → credited catch → .
amateescu → credited catch → .
This looks very nifty. I left one small comment on the MR.
@akulsaxena there's not enough in this issue for an RTBC to count towards issue credit, see https://www.drupal.org/about/core/policies/maintainers/how-is-credit-gra... →
Committed/pushed to 11.x and cherry-picked to 11.1.x thanks!
These are the current settings:
executables:
composer: ~
rsync: ~
additional_trusted_composer_plugins: []
include_unknown_files_in_project_root: false
The composer and rsync executables could be environment specific, so for me those should definitely move to settings.php to avoid people having to use $config overrides in settings.php (which is less obvious than $settings).
The other two also seem like they could be environment specific potentially.
If Acquia is injecting settings into settings.php (via environment variables, an include etc.), it could be set there.
Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!
Needs a rebase.
Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!
Translated JavaScript files are always stored in the public files directory:
$dir = 'public://' . \Drupal::config('locale.settings')->get('javascript.directory');
As I said already in #19:
It might be worth a new issue to discuss moving generated JavaScript translation files to the assets:// stream wrapper so they can be kept out of s3
But that should be a new, clearly defined, task against Drupal core - and it's not yet been demonstrated that this is the cause of the performance problem described here.
For the record I don't have access to change user roles on Drupal.org so I am unable to give people the role under any process, exceptions or not.
With this I am not saying that no issue must be created. An issue should still be created (for transparency), but it would be handled considering the role that person already has.
So would you accept something like:
'If you are already a core subsystem or topic maintainer, you should still open an issue but just add documentation of your Drupal core role'?
But this does not help someone who has been contributing to core for years and is not listed in MAINTAINERS.txt - there are dozens or hundreds of people like that, and they would need something closer to what is in the issue summary already.
Checking that a set of patches have been effectively done by the applicant, and that the applicant is effectively a significant contributor, is not a moment for teaching or sharing knowledge.
No but we're talking about people who have already got years of Drupal contribution under their belt who don't have this role yet - this entire issue is trying to find a process that works for those people - and historically there has been a lot of them - I didn't have cvs access until I'd been contributing to core for several years because I just did not need it. Then someone wanted to make me a co-maintainer of a module, couldn't do so, then I was given it by someone later that day. If I'd needed to go through the current process, it could have been several years longer.
I went through the modules from http://codcontrib.hank.vps-private.net/search?text=-%20field.storage.nod... alphabetically up to E. Not a single module had a stable or even dev release for Drupal 10 or Drupal 11.
Out of 14 projects, the combined usage was 15. 10 usages are from an install profile. So in fact the 13 actual modules have a combined usage of 5. Some of them have Drupal 7 releases and no Drupal 8 releases which could mean the usage is even lower than 5.
Install profiles can very easily add node body field storage same as we've done for the standard profile in this MR - I think this is reasonable with a change record.
Book and blog do ship node types with body fields, however they can add the storage field to config/optional and/or book ships its config as optional anyway. Blog module doesn't have Drupal 11 support so will need to be updated anyway before it can be installed on new sites. Book module has supported any configured content type since Drupal 6, so the shipped node type is really vestigial and could be dropped too.
Maybe there is a high usage contrib module between E-Z in that list, but even for that, the change would only impact installs on new sites, not existing sites which already have the module installed and control their own config anyway, so I think the potential disruption here is pretty low.
10 second+ response times up from 1+ second is critical. Do we know if there's already an upstream issue to track this?
@alexpott I never use the core body field on sites, not for 15 years or more, because it uses text_with_summary, and have never noticed a contrib module relying on it. Do you have an example other than tests?
We can make node_add_body_field() more robust but also I doubt any code outside core calls it.
@iselectweb please try to post steps to reproduce this issue from a clean Drupal install, it has taken 18 comments to find out that you're using s3fs module.
JavaScript translations are created on demand, so if s3fs is storing them, that may not be helping.
One issue related to this is 📌 Remove on-demand JavaScript translation parsing and do everything on rebuild Needs work .
It might be worth a new issue to discuss moving generated JavaScript translation files to the assets:// stream wrapper so they can be kept out of s3 - see https://www.drupal.org/node/3328126 → for details.