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.
If there is an SSRF or other security vulnerability this should be reported to the security team instead of being handled in public.
Thanks everyone, there is nothing left to do here that I can see, so marking fixed.
Should we also drop mentions of Percona from the installer here? There are known Percona bugs ( 🐛 Lock issues when clearing cache on Percona XtraDB cluster Needs work ), and several other MySQL-compatible engines that we don't mention.
longwave → made their first commit to this issue’s fork.
Also the bot doing this has a point:
Checking core/tests/Drupal/Tests/Composer/fixtures/fake-composer/bin/composer
check failed: file core/tests/Drupal/Tests/Composer/fixtures/fake-composer/bin/composer should not be executable
The core commit check script ensures all files are not executable, so we need an exception or some other way to set this up.
Saving credit for discussions.
We added explicit detection of MariaDB in #3109534: Raise the minimum MySQL version to 5.7.8 and MariaDB version to 10.2.7 in Drupal 9 → similar to the suggestion in #3, closing this as outdated - unless there is a good reason to split them in future we should try to keep all MySQL-compatible engines in the same driver.
Is there a case for making this configurable, at least with an on/off flag, for sites that will never use automatic updates and want to retain the existing behaviour?
A protected property that is considered @internal should probably be private instead? Whether we can change this now, however...
🐛 Canonical taxonomy term link for forum vocabulary is broken Postponed reminded me of this; forum is removing use of uri_callback, once comment does as well then it can be removed from core entirely.
Rebased the MR, updated the deprecation for 11.3, updated issue credit, I think this is ready to go.
Removed these from the IS as they already exist as followups:
📌
Rename Comment::permalink() to not be ambiguous with ::uri()
Needs work
📌
Deprecate uri_callback in routes for entities
Needs work
longwave → made their first commit to this issue’s fork.
Some questions:
- Is there a parent meta, because the parent/child links here and in the related issues seem to go round in circles?
- Is there an issue for updating the docs at https://www.drupal.org/node/2116043 → ?
- I agree with #4, happy to advocate for optional method docblocks for tests if there is an issue for that.
- Do we have a longer term plan for
@legacy-covers? It seems unlikely that upstream will change their mind on the matter.
Agree with #3, if someone wants to maintain a contributed module that contains BC shims for previous versions of jQuery then they are free to do so, but this doesn't fit with our BC policy for core; we are trying to minimise JavaScript and so don't want to load backward compatibility code that probably won't be used on 99% of sites.
longwave → created an issue.
Backported down to 10.5.x as a critical regression, this only affects the link and not the translated string.
Committed and pushed 054568a1f05 to 11.x and fee3dc830b3 to 11.2.x and d1db7dc0cb6 to 10.6.x and d731332ecfc to 10.5.x. Thanks!
Is it possible to get a full stack trace from where that exception happens? The update hook itself doesn't use TranslatableMarkup so it's not clear to me how or why this happens.
Thanks for working on this. As a cleanup task this is not worth backporting, committed to 11.x only.
Committed 0951bca and pushed to 11.x. Thanks!
Great work here - nice to see this finally land! Tagging as a release highlight and will publish the change record.
Committed e117838 and pushed to 11.x. Thanks!
I tried to credit everyone who provided code changes or helpful comments that moved the issue along in some way; I did not credit those who just uploaded patches, because we should be using MR workflows now. Apologies if I missed anyone.
Better title :)
Traced this back to an issue in the original config_translation module, where nl2br() was explicitly added to fix a bug:
#1945184: Original value display is broken →
Therefore I think we should keep this feature? (and probably add a test for it...)
Do we know why the nl2br() was originally there? Do we need to care about representing newlines in the string correctly still?