It just struck me that values here is just an entire process pipeline, and get is the default, so perhaps we can do away with the loop entirely!
This needs work on docs (and tests) but a custom plugin with this code that extends entity_generate works for me.
longwave → created an issue.
longwave → created an issue.
smustgrave → credited longwave → .
longwave → created an issue.
longwave → created an issue.
benjifisher → credited longwave → .
#30 still stands today, the weirdness is that the constants are defined in core then used once in file.module and once in Views, but these uses could probably be swapped to self-contained constants in each of those cases; it doesn't make much sense to share them. Neither of the things in #30 has happened in the time since that was posted, though. Maybe we just make those issues, postpone this on them happening, and wait another ten years?
smustgrave → credited longwave → .
@luenemann it makes no sense to me why the linkset functionality was merged directly into system.module and not as a separate module, it would simplify the code as the routing and controller would be self-contained in their own module.
I guess the question is: what is the difference between a full module and a feature flag? If the feature flag's code can be contained within a module itself, where do we draw the line? Even in core we have things like Syslog and Telephone which are very small and self contained, but they are still separate modules.
Discussed with @catch. drupal/core also specifies a minimum PHP version, but composer create-project must ignore this. We discussed adding it to the drupal/core-recommended metapackage but probably the most reliable option is to add it to the project template, which is what Symfony and Laravel also do.
Let's try to find a way to prevent this by adding a constraint somewhere, so Composer gives a more useful error message.
Can reproduce locally:
$ php8.2 /usr/local/bin/composer create-project drupal/recommended-project:11.2.2
Creating a "drupal/recommended-project:11.2.2" project at "./recommended-project"
Installing drupal/recommended-project (11.2.2)
- Downloading drupal/recommended-project (11.2.2)
- Installing drupal/recommended-project (11.2.2): Extracting archive
Created project in /tmp/recommended-project
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Package operations: 64 installs, 0 updates, 0 removals
...
- Installing composer/installers (v2.3.0): Extracting archive
- Installing drupal/core-composer-scaffold (11.2.2): Extracting archive
- Installing drupal/core-project-message (11.2.2): Extracting archive
- Installing drupal/core-recipe-unpack (11.2.2): Extracting archive
Install of drupal/core-recipe-unpack failed
In Plugin.php line 28:
[ParseError]
syntax error, unexpected identifier "RECIPE_PACKAGE_TYPE", expecting "="
Exception trace:
at /tmp/recommended-project/vendor/drupal/core-recipe-unpack/Plugin.php:28
PHP 8.2.29 & 10.11.11-MariaDB
commad is used : composer create-project drupal/recommended-project:11.2.2 ./
Drupal 11 requires PHP 8.3: https://www.drupal.org/docs/getting-started/system-requirements/overview... →
I wonder if there is something we should add to drupal/recommended-project or another dependency to prevent Composer from trying to install on an earlier version.
I think we should keep all of them. Existing code might typehint on any of the available names in the hierarchy, and we should allow autowiring on any of them.
hook_update_N() is not run at module install time either, so the suggested comment is not quite right.
Stark is perhaps useful for the tests as the markup is very simple, maybe we just need to install the theme in a different way in these tests (unless the tests are testing the theme installer itself)
Yeah the module installer now takes arrays of modules so this is done, let's close as outdated.
This is long obsolete since #2352155: Remove HtmlFragment/HtmlPage → , the ideas from there have not returned, let's close this out.
Fixing tags
Looks good to me. Let's also ship this in 11.2.3 to hopefully fix the problems reported in this thread. Removing RM tag, hopefully @catch can remove the FM tag?
As for the @todo let's revisit when PHPUnit 10 support is removed but given the small API surface maybe we are okay with keeping this in place.
Yeah, it's never been clear to me where this sort of thing should go. Right now we have an awkward combination of:
$settingsarray in settings.php$configarray in settings.php- config form with a UI
- container parameter in services.yml
- feature flag module
- some new API? ✨ Add an API for feature flags Active
Also ideally it wouldn't be a moduleExists() check, instead the code to handle this feature would live self-contained in the contrib module, extending basic_auth as necessary.
Let's try out adding the annotation and seeing what the impact is. If we have to add a bunch of stuff to the baseline, so be it; perhaps we can figure a way of ignoring it for core only, but letting downstream users get their notifications.
Answering my own question:
v5 changes the constructor - we already supported this as far as I can see.
v6 removes some methods - we don't use any of these - and drops PHP 8.1 support.
v7 drops PHP 8.3 support, so we can't lock to this.
So increasing the constraint seems fine as long as we are careful with the lockfile.
That depends on what the breaking changes are in 4 and 7, though it seems unlikely we are affected given we only use this dependency in a very limited capacity.
Added 📌 Update CKEditor 5 to 46.0.0 Active for 11.3.
10.6 should not need to follow, because I believe CKEditor have agreed to supporting v45 until the end of life of Drupal 10.
longwave → created an issue.
Looks good to me.
longwave → created an issue.
longwave → created an issue.
@thejimbirch what is worthy of mentioning in the release notes here? There is no snippet in the IS and we usually don't call out bug fixes unless they affect many users in some way.
Instead of having a settings.php entry, how about a submodule, e.g. "Basic Auth with Site Lock" (the name probably needs work) - this would act as the flag instead? The module description can explain why you might want this enabled if you are using Basic Auth.
> The basic auth provider will only act upon routes which have been explicitly tagged with the basic_auth _auth option.
If basic_auth is enabled, and basic auth is received on a route that is *not* tagged for basic auth, should we log or display a warning message?
Historically there's a lot of things that we have always intended to do in followups, but several times they have been forgotten or derailed and so don't happen in a reasonable timeframe. Sometimes that is okay, but in this case I think it's valid to do it differently.
Needs work for the change record.
If we inject the container and read from it at runtime I think this means we will never be able to make event listener services private by default. I am not sure if this is important.
The problem is when can we make these changes. The bigger the change the more likely we have to wait for 11.3 but given this is a regression maybe we want a stopgap fix in 11.2?
longwave → changed the visibility of the branch 3538212-investigate-reports-of to hidden.
longwave → changed the visibility of the branch 3538212-combine-calls to hidden.
It's the Umami homepage as anonymous, so on a warm cache no hooks should be invoked, hence the drop in calls to addListener. As I understand it from the Reddit thread and the issue summary it's cached pages that are the problem here?
As soon as a hook is invoked I think this will make no difference, because instantiating the hook event dispatcher goes back to being expensive.
Yeah I was surprised how small that change was, but it makes sense now I look at the bigger picture - we are using the event dispatcher only as data storage and we have to augment it with a container parameter anyway, so it does make sense to use alternative storage that actually meets our needs.
Discovered in 📌 Investigate reports of performance drop in 11.1 Active , can we just have two event dispatchers? The default one, and then a separate one for hooks, which we only inject into ModuleHandler?
https://git.drupalcode.org/project/drupal/-/merge_requests/12850 implements this in order to try and avoid warm cache performance issues reported in that issue.
MR!12850 is promising locally. Testing on the Umami homepage, warm cache:
11.x:
Wall Time 11,910 µs
CPU Time 9,726 µs
Memory Usage 3,249,088 bytes
Peak Memory Usage 4,462,840 bytes
670 calls to EventDispatcher::addListener()
MR!12850:
Wall Time 8,083 µs
CPU Time 6,179 µs
Memory Usage 2,082,648 bytes
Peak Memory Usage 2,481,720 bytes
151 calls to EventDispatcher::addListener()
This MR combines the calls to addListener() which removes ~670 method calls locally, but it doesn't really make that much difference.
There is nothing stopping us having multiple event dispatchers; I am wondering if we should put hooks on their own, separate dispatcher.
catch is good at predicting the future: #2909185-103: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher →
This would be a BC break for anyone relying on it in the current state, we could go through the hoops of deprecating and changing it somehow but it doesn't seem like it's worth the effort.
Re #6/#12 this is a security feature, because the filter format may previously have sanitised some HTML, and once that is removed we can no longer safely display the content. I believe this is also why deleting a format from the UI is not allowed either.
This makes me sad:
- $node_type->setPreviewMode(DRUPAL_REQUIRED);
+ $node_type->setPreviewMode(NodePreview::Required->value);
Once we have enums we shouldn't care about ->value, ideally we want to just pass the constant around? ie.
- $node_type->setPreviewMode(DRUPAL_REQUIRED);
+ $node_type->setPreviewMode(NodePreview::Required);
Let's ask the framework managers for an opinion.
I had not seen this issue before but I think it is worth considering. Both the quoted true and the leading underscore feels non standard and confusing to new users, so if we could allow both it would be a DX improvement.
@cboyden is there a reason you can't upgrade to Drupal 11? 10.6 is a maintenance minor release where we try to minimise changes to ensure stability; the criteria for backports is listed at https://www.drupal.org/about/core/policies/core-change-policies/allowed-... → - as a feature request this doesn't appear to fit the criteria.
As #26 says, you can't just add a new argument to the constructor, especially in the middle of the argument list; contrib or custom code may extend this class - for example I'm fairly sure Ultimate Cron in contrib extends this service. If we are going to do that we need to add it to the end of the list and provide backward compatibility.
We don't need to make this so complicated. Given that we already do
$time_logging_enabled = \Drupal::config('system.cron')->get('logging');
then I think it's fine to copy-paste that to run() and be done here; we can handle injecting the config factory as a dependency in another issue.
> The options of an URL object are documented as optional, which means NULL is a valid value.
It doesn't mean NULL is valid, because the default $options is an empty array. Eventually we will be adding types and this argument will be typed as an array.
This means it is the caller's responsibility to ensure an array is passed in. In the LinkFormatter case, the underlying options property in LinkItem is a Map, which is stored either as an array of values or NULL. Therefore I think the fix should be in LinkFormatter::buildUrl():
$options = $item->options ?? [];
No further comments, this is as small a change as can be to get this feature in. Marking RTBC for another committer to double check everything.
From a security point of view I think this is fine, given it's new behaviour that is explicitly opt-in, so existing sites don't have to change anything. Site owners can do slightly more dangerous things with this feature but that's up to them to configure it and it's not up to us to prevent that.
I don't see any release management issues either given there is little disruption here. The only possible minor disruption is the addition of the File constraint which there is a chance is already used in the wild somewhere, though given this just uses the Symfony constraint it is likely they have made the same implementation anyway. We should add a change record for this just in case.
We should also document this new capability in composer/Plugin/VendorHardening/README.txt, marking NW for that and the CR.
Also removing the bot tag as I think the bot was correct earlier when we had that executable file in the patch, that is gone so the bot should be happy.
#28 will break if someone switches hosting or PHP version and suddenly Argon2 is no longer available, I am not sure we should do this.
Both Symfony and WordPress base64 encode the output of the pre-hash function, which is the mitigation for the NULL byte problem discussed in that article.
In one way I think we should follow Symfony, given that we use other Symfony components it might make sense for us to switch to the Symfony PasswordHasher component one day instead of managing this ourselves; if we use compatible code then that helps us to switch in future. But that does not follow the best practice identified by @mcdruid in #20...
Symfony does SHA512 followed by base64:
base64_encode(hash('sha512', $password, true));
This is actually 88 bytes long, but the Symfony maintainers considered it secure enough for bcrypt to then truncate to 72 bytes.
WordPress does SHA384 with a fixed pepper of wp-sha384 followed by base64.
base64_encode(hash_hmac('sha384', trim($password), 'wp-sha384', true));
This results in a 64 byte string.
In both cases the base64 encoding is required because the raw hash may contain a NULL 0x00 character, which bcrypt cannot handle:
php > print password_hash(chr(0), PASSWORD_BCRYPT);
PHP Warning: Uncaught ValueError: Bcrypt password must not contain null character in php shell code:1
Given Symfony and WordPress pre-hash longer passwords then we could follow that? Then we avoid any UX issues; the only difficulty is backward compatibility in identifying pre-hashed vs non-pre-hashed for existing passwords over 72 bytes, but WordPress handles this with a known prefix on the hashed string.
Adding some other reference points:
Symfony hashes passwords longer than 72 bytes with sha512 before hashing with bcrypt: https://github.com/symfony/symfony/pull/40920
WordPress hashes passwords longer than 72 bytes with sha384 before hashing with bcrypt: https://github.com/WordPress/wordpress-develop/pull/7333
📌 Remove remaining IE11 support from Olivero Active landed so I think we are done here - setting to NR for someone to check that I haven't missed anything.
Usually we don't scope things by module, I think this is a duplicate of 📌 Autowire core modules that do not require explicit configuration Postponed
But in practical terms, doesn't that emoji have far more entropy and hence is much more secure than a single ASCII letter, surely? Nobody can type that with a single keystroke and even if they saw it displayed or printed they would be unlikely to be able to reproduce it, therefore to me they are not really comparable.
While researching this issue I found that GitLab had the same problem reported several years ago, which is still open:
If we don't like #4 then an alternative is to add a new password implementation that hashes the password with e.g. sha256 before calling bcrypt (or perhaps we only hash if the password is over 72 characters?). Some sources recommend this and some applications have implemented it, while others recommend against it.
In PHP it turns out that this has been known about since 2013 - see https://stackoverflow.com/questions/16594613/how-to-hash-long-passwords-... - and phpass is suggested as a viable alternative!
Refusing to hash a password longer than 72 characters is what Spring decided to do for CVE-2025-22228: https://github.com/spring-projects/spring-security/commit/46f0dc6dfc8402...
Further discussion about a regression that this caused: https://github.com/spring-projects/spring-security/issues/16802
I don't think we should be changing password algorithms again. password_hash() is only guaranteed to implement bcrypt, Argon2 is a compile time option and we cannot guarantee it is available. Therefore to switch to another algorithm we would need a new dependency, or as a last resort, swap back to phpass.
I also don't think that in practice the 72 character limit is an issue for most users or sites, even with password managers in widespread use.
I do think the suggested proposal is acceptable, given that we have both hash() and check() methods on PasswordInterface.
Callers to hash() for newly stored passwords will reject the password if it doesn't fit the criteria. If we limit this check to bcrypt then users who have configured Argon, or any future password algorithm, will not be affected.
Callers to check() will retain backward compatibility because that will still accept longer passwords; only new passwords will be affected.
We currently enforce maxlength 128 on password elements. I don't think we should change this here. We should however test the UX when bcrypt is in use and a password between 73 and 128 characters is attempted to be stored.
I'm not sure we should commit at least the text-align-* part, as I think this change affects RTL users who might have formatted text in CKEditor. The CKEditor alignment plugin uses these classes:
ckeditor5_alignment:
ckeditor5:
plugins: [alignment.Alignment]
config:
# @see core/modules/system/css/components/align.module.css
alignment:
options:
- name: left
className: text-align-left
- name: center
className: text-align-center
- name: right
className: text-align-right
- name: justify
className: text-align-justify
If users in an RTL language have used these to explicitly force text to one of the screen, after this is committed, won't the text move to the other side? It also means the icons are backwards for RTL users now; the icons that shows left-aligned text will right align it for them.
Personally I think the new nesting style is less readable in compiled form, but it is what it is, so let's get this in. Not eligible for backport due to the risk of regressions.
Committed d0e9947 and pushed to 11.x. Thanks!
Was about to commit but the check script failed
FILE: .../core/modules/node/tests/src/Functional/Views/NodeLanguageTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
95 | ERROR | The array declaration extends to column 126 (the limit is 120).
| | The array content should be split up over multiple lines
| | (Drupal.Arrays.Array.LongLineDeclaration)
--------------------------------------------------------------------------------
I can think of slightly neater ways of writing this code but as this is an edge case mostly only used by @catch then let's just ship it and move on :)
Committed 57f7daa and pushed to 11.x. Thanks!
This is eligible for backport as an accessibility bug fix, but does not apply cleanly to 10.6.x. If anyone wants to backport this down to 10.x please open a backport MR; otherwise we will just fix this in 11.x and move on.
Committed and pushed e7013351307 to 11.x and dadcf295d0c to 11.2.x. Thanks!
For changes like this, screenshots are only helpful if they show both before and after, so I have not credited anyone that only uploaded "after" screenshots.
This is probably out of scope and requires a new issue, but for users with permission, should we just allow them to configure these settings directly from the block?
I manually tested this but there is a bug, the "Theme Settings" link does not always take me to the right place.
Committed a simplification, but added some comments about additional test cases.
longwave → made their first commit to this issue’s fork.
The DA has prevented certain projects, including most core module names, from being created on d.o: https://bitbucket.org/drupalorg-infrastructure/drupal.org/src/6e4bceb300...
Create an issue at
https://www.drupal.org/project/infrastructure →
linking back to here to get ban unbanned.
Managed to crosspost with @xjm while we were both reviewing, committed her comment improvement as a followup.
@acbramley thanks for the explanation, it did feel like the Umami change was too good to be true here, and it turns out that was correct; glad we did not break that - and technically it is tested here indirectly, because we spotted it via the performance tests!
So, after 14 years and 240 comments by many contributors, I am happy to finally be committing what is really a one line change:
--- a/core/modules/node/src/Entity/Node.php
+++ b/core/modules/node/src/Entity/Node.php
@@ -357,7 +357,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
->setLabel(t('Promoted to front page'))
->setRevisionable(TRUE)
->setTranslatable(TRUE)
- ->setDefaultValue(TRUE)
+ ->setDefaultValue(FALSE)
One small step towards (possibly) removing this feature in the future!
Committed 6bfec5b and pushed to 11.x. Thanks!
Also finalised and published the change record.
Given the speed of the JavaScript ecosystem I think any attempt to support something like this is likely better off in contrib, unless there is a clear winner that we could support in core - right now I assume that doesn't exist.
Anyone is free to create such a contrib module, and if it gains traction it may be possible to move it to core eventually. Until then I'm closing this issue as won't fix.
core-recommended also requires sebastian/diff 6, but PHPUnit 10 requires sebastian/diff 5, so anyone using core-recommended will be forced to upgrade to PHPUnit 11.
🐛 Can't update to 11.2.0 Active will resolve that so core-recommended users should be able to use either version.
fjgarlin → credited longwave → .
As with the SSRF issue that was opened, if there is a potential security issue this should be reported to the security team in private - this should not be discussed in public.
Overall this looks good but I think we can remove these overrides now?
core/profiles/demo_umami/config/install/core.base_field_override.node.page.promote.yml
core/profiles/standard/config/install/core.base_field_override.node.page.promote.yml
core/recipes/page_content_type/config/core.base_field_override.node.page.promote.yml
These seem to exist to set the promote default for these content types to false - but it's now false by default, so we shouldn't need to ship them any more?
Also I'm intrigued why the Umami performance stats have such an improvement; I don't see why just flipping the default results in so many fewer queries?
Found a couple of nits.
Backported down to 10.5.x as a major bug fix.
Committed and pushed 8d7e0377abb to 11.x and d4d050cdb23 to 11.2.x and d9ba7c3c96c to 10.6.x and 2f7179b2a41 to 10.5.x. Thanks!
Thanks! There was no test for the nl2br() before, let's not hold this up on adding one.