Committed and pushed e4a89e5ef80 to 10.6.x and f1f81bba162 to 10.5.x. Thanks!
We have EditorialContentEntityBase and the generic revision UI now, not sure what else we need to do here.
Things have moved on a lot, with the current work of moving the hooks to OOP and issues like 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active I think this can be closed as outdated.
Only one person requested this, over eight years ago. This is a niche feature that I don't think we need to support in core given the lack of interest, therefore closing this one out.
Thanks for the swift turnaround, let's try this again.
Committed e2aea6e and pushed to 11.x. Thanks!
I am still in favour of doing this as per #39 and #45.
#11 is 5 years old, referencing a blog post from 8 years ago; things have moved on since then.
This is a nice cleanup. Doesn't backport cleanly to 10.x, not sure it is worth the additional effort.
Committed f7bf351 and pushed to 11.x. Thanks!
Given this is a slight behaviour change as discussed in #17 through #26 I think this is best to commit to a minor only.
Committed and pushed 386cfa84bdb to 11.x. Thanks!
Looks good to me too. As it contains a database update this is only eligible for 11.x.
Committed b500857 and pushed to 11.x. Thanks!
Fixed up the docs, removed database drivers for now, and I wrote a basic change record at https://www.drupal.org/node/3549907 →
Yep the screenshot in #3 is saying you can't upgrade to symfony/console 7.3.0 because chi-teck/drupal-code-generator isn't compatible - this is a dependency of Drush, you can either remove Drush or try to upgrade it at the same time.
Composer is telling you that something else requires symfony/console 7.3.0. Try composer why-not symfony/console 7.3.0 which might give you additional clues as to what that is, or if you post your composer.json someone might be able to figure it out.
Fixed up a couple of comments and added types to ::theme().
Removed ThemeEngineInterface::getExtension(), the theme engine is directly responsible for appending file extensions now.
Otherwise, are we done here? Or is there more preprocess to deprecate somewhere?
I think this is just a duplicate of 🐛 RowPluginBase::render() update docblock. Needs work
Tagging as a highlight, improving performance is always good.
This is getting super tricky to deprecate extension.list.theme_engine while still actually needing to use it for various tests, maybe have to revert this and leave it as an unused service that we can just quietly remove in Drupal 13.
Yeah it's not clear in many cases if we should be resetting one, some, or all. Not sure if we need to mess with any of that here though, if there are no known bugs; in practice the lists do not change often.
Following #81 the prefix property on theme value objects is used in a couple of tests, and more importantly ThemeSettingsForm, where we have another magic function name:
if ($theme) {
// Call engine-specific settings.
$function = $themes[$theme]->prefix . '_engine_settings';
if (function_exists($function)) {
I think we can just drop this with no replacement? I had no idea this existed, and if there are custom theme engines somewhere in the wild then they can alter this form in their new custom module instead now.
Unsure if or how to provide BC in the constructor here, but I also don't see why anyone would override this.
longwave → made their first commit to this issue’s fork.
Oh, #2973439: Make extension services tagged services → already exists for similar reasons to #81.
Started looking into ThemeEngineExtensionList aka extension.list.theme_engine.
It's injected into extension.list.theme where it's used to populate the owner and prefix properties of theme value objects. owner is the pathname to the .engine file, which will go away once these are services, and prefix is the engine name itself, but I don't see where this is actually used so hopefully it can just be dropped.
It's also injected into extension.path.resolver (as are all the extension lists) but I don't think the engine list is ever used in core from here. I think extension list services should be similarly converted to tagged services and then the path resolver can pick them up using a service locator; that way, contrib could add another extension type if they really wanted to (though this seems super edge cases) but also the lists will be instantiated on demand instead of at injection time. Will open a sister issue to handle this.
I guess we also should deprecate ThemeEngineExtensionList while we're here?
Added a deprecation for .engine files.
I'm confused by the second half of ::loadActiveTheme(), when does getEngine() return null/empty string and trigger this code?
else {
// Include non-engine theme files.
foreach (array_reverse($active_theme->getBaseThemeExtensions()) as $base) {
// Include the theme file or the engine.
if ($base->owner) {
include_once $this->root . '/' . $base->owner;
}
}
// And our theme gets one too.
if ($active_theme->getOwner()) {
include_once $this->root . '/' . $active_theme->getOwner();
}
}
Should we open a related issue to deprecate "base theme engines" - this doesn't seem used anyway and now theme engines are services they can just extend a parent service.
Ran out of time yesterday so I pushed and hoped, should get time to fix it up later today.
longwave → made their first commit to this issue’s fork.
Great find. Certainly undeniable that there is a performance improvement here!
This prevents the event dispatcher from being instantiated when a page is delivered from the cache.
Could/should we add an explicit test for this exact case to prevent a regression? All we would need to break this again is remove the \Closure type in the PageCache constructor, I think. We deleted testLazyLateMiddlewares() because it's broken and wasn't testing what we thought it was, but perhaps we should port the underlying intent to this new way of doing things.
Only one further comment was received, agreeing that they had not noticed any difference with PHPUnit 11. Given there has been no disagreement to the plan I think it's fine to move forwards with this.
Agree this doesn't need tests as a trivial fix.
Committed and pushed 51dbf7b4845 to 10.6.x and 81fd798bedd to 10.5.x. Thanks!
Appears to be a duplicate of https://github.com/composer/installers/issues/544
One minor simplification that I am equally happy to skip, otherwise this looks good to me, but I am not eligible to commit as I worked on it.
https://www.drupal.org/project/email → is a D7 only module that was merged into core for D8.
Trying to find something short and simple here, because I think mailer_symfony is both long-winded and could easily be confused with symfony_mailer.
I asked in #maintainers in Slack, and those who responded were largely unaware that we had moved to PHPUnit 11 even, but their code was still working fine, or they had had to make minimal changes. I think we should give it a few more days but it seems so far that dropping PHPUnit 10 support won't be a problem even though it is technically a BC break.
I will credit those who contributed to the discussion.
The only commits since 2017 are Drupal 9 and 10 compatibility fixes, there are no published compatible releases and it is "seeking new maintainers", next week I will contact the maintainers and ask.
What about just mail for the core module?
https://www.drupal.org/project/mail → exists but seems to be abandoned, we could ask the maintainers whether we could take over the namespace?
I'm not 100% happy with the "AutowireArguments" name, if anyone has better suggestions feel free to offer them!
We aren't yet sure if we will have full compatibility for PHP 8.5 in Drupal 10 (our dependencies may not allow it) but I don't see the harm in backporting things like this.
We are adding the custom messages feature here so let's have a change record for that. In a way I wish this was split into two issues but I guess we can handle both the version change and the custom messages thing here.
smustgrave → credited longwave → .
I think everything in Composer is testable locally with path repos, the tricky part is setting it all up correctly to match what would actually happen with packages.drupal.org.
As I understand it ✨ Introduce kernel parameters allowing to specify password hashing algorithm and options Active will add a new warning for all site owners until they make a manual change? To me this is a terrible UX - if we have to keep bcrypt as the default algorithm then I think we should follow Symfony and other projects by using the hashing technique - otherwise, every site owner has to either understand this issue or live with a warning forever?
If core uses the composer.json replace property to say that core now includes Gin, and the facade continues to point to the contrib version, will that work? Existing versions of core will not see any difference, newer versions of core will tell Composer that they already include Gin so the contrib version is not required?
@fjgarlin is it possible to add the noreply address by default for each co-author as well? I think that is required for GitLab to link the commits back to users - assuming those addresses are known on the GitLab side, I can't see my GitLab profile directly so not sure if this is set up or not.
FWIW WordPress's Gutenberg repo also uses Co-authored-by to credit all contributors to a PR: https://make.wordpress.org/core/2024/02/01/new-commit-message-requiremen...
These lines are officially called "trailers" and I think there are some vague standards for them.
GitLab supports certain forms of these, though the docs are sparse: https://docs.gitlab.com/user/project/repository/signed_commits/web_commits/
GitHub supports at least Co-authored-by: https://docs.github.com/en/pull-requests/committing-changes-to-your-proj...
Would it be helpful for future git archaelogy if we used a somewhat supported standard here?
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.