So there might be a new problem this issue has surfaced -- The deprecation warning appears to be gone in 10.3/4 but gone in Drupal 11. Doesn't that mean when you're running the deprecation checks, you won't see this warning, until the module WSODs with Drupal 11?
https://git.drupalcode.org/project/drupal/-/blob/10.3.x/core/lib/Drupal/...
vs
https://git.drupalcode.org/project/drupal/-/blob/11.0.x/core/lib/Drupal/...
Will be handled in https://www.drupal.org/project/acquia_search/issues/3450845 📌 Automated Drupal 11 compatibility fixes for acquia_search Needs review
Will be handled in https://www.drupal.org/project/acquia_search/issues/3450845 📌 Automated Drupal 11 compatibility fixes for acquia_search Needs review
The 4.0 to 4.1 update is really an incremental one, sorta like 4.0.4 to 4.0.5. But its unfortunate that Drupal core treats minor releases of contrib modules like core, because any old release is instantly 'unsupported', but there is only an alert when its a minor version update. I wasn't going to make a 4.0.5 release, but then noticed that if someone had a blocking issue with 10.3 and was 'pinned' to the 4.0.x releases, we should fix it there so the 3.x/4.0 releases remained D10 compatible.
So if you're on the 4.x releases, updating to 4.1 should be seamless, unless a downstream module pinned to ~4.0.4, which is not a recommended practice.
As for 3.x, it was marked as 'supported' but not recommended to give a little more time for maintainers with downstream modules so they can update their dependencies to 4.x from 3.x. But 3.x is also essentially EOL and will be marked unsupported sometime after D11 comes out.
The gitlab ci issue should be committed and tested before this gets committed. With the changes to return types its very unlikely this will work in Drupal 8, so the minmum version should be updated to Drupal 9.5.11
Looking at the changes, IMHO the minimum should be 9.5.11 and not 10.2. There is no reason to artificially squeeze the compatibility window.
gitlabci should be added (either here or a separate issue) to ensure tests are actually passing before this gets committed.
The tests are failing in head, but its possible thats due to current incompatibilities, which would be made worse with D11 support. Those should get fixed before this gets committed.
gitlab ci needs to be added and verify tests passing before it can be merged.
gitlab.ci needs to be added and tests passing before we merge this.
Cannot merge as there is a conflict, also the minimum Drupal version should be increased to 9.5.11
Tests are failing
Working on it. Theres a few other page manager things I gotta look at today as well, but will prioritize this since its breaking builds. In the meantime, you can pin ctools to 4.0.4.
Both 8.x-3.15 and 4.1.0 (recommended) are now out!
4.1.0 is out! with this, 4.0.5 and 8.x-3.15 are the final releases for D10 and below... unless there is a security or other fatal issue found.
All future development should be against 4.1.x.
japerry → made their first commit to this issue’s fork.
Committed to 4.x!
japerry → changed the visibility of the branch 3392227-minor-coding-fixes to active.
japerry → changed the visibility of the branch 3392227-minor-coding-fixes to hidden.
Spent a little time on this today. While I could not reproduce the issue, there isn't really a reason to be injecting these services other than phpstan. Therefore I reverted the change and pushed to head in 1.0.x and 1.1
ugh, sorry about that. Re-committed to both 1.0 and 1.1 branches
Good find, I was also having this issue while testing 10.3. Apparently in our tests, if the 'valid' image formats service had its config set, the test couldn't change it later.
Fixed!
Failing 10.3 tests for other reasons. Committing this now so I can get the 10.3 fixes in later.
It just sounds like the service container needs to be reloaded. its crashing when you run drush cr?
Reviewed with eclipsegc, and the MR should fix this issue. There might be random issues for sites upgrading, for those cases please open a new issue
Looks fine to me, committed!
japerry → made their first commit to this issue’s fork.
reverted my removal of Drupal 9. 4.1.0 can be D9 compatible, and we can drop 9.5 later if the code requires it. Tests will only work in Drupal 10.2+, which is fine.
Moving to 4.x as thats where we full support Drupal 11.
+1 to the event subscriber! Regarding the config, yah thats what mglaman suggested to do as well, that should be fine for testing.
Updated the parent summary. Basically:
- 8.x-3.x will NOT support Drupal 11
- 4.x will become the base development branch
- 4.1.0 will be Drupal 10 and 11 compatible
There is an orca issue with next minor, but doesn't seem to be related to the module.. so committed!
Changing the eventdispatcher interface requires removing support for Drupal versions earlier than 9.5. (which is fine..) see if tests run now.
Don't go making major version changes
I have major issues with the conclusion in Matt's blog post, as made by comment #12. I cannot understate the problems caused by making new major releases, especially if you have a dependent module ecosystem. Think about Drupal core: every 2 years or so the community has to make this massive change to every module to remain compatible with Drupal core. By adopting the suggestion in Matt's blog, every contrib module and site has to not only become compatible with Drupal core, but with all the other contrib modules. Instead of one target (core), you're chasing multiple targets (your dependencies) that all update at different timeframes.
Additionally, your major version now is tied to Drupal core and not your API, and if you're making major API/architectural changes, you now have another set of major versions to maintain.
Bad:
Within 3 years, you could see the following:
layout_builder_restrictions 8.x-2.x --> Drupal 9.3, 10
layout_builder_restrictions 3.x -> Drupal 10, 11
layout_builder_restrictions 4.x -> Drupal 10, 11 --> New API changes
layout_builder_restrictions 5.x -> Drupal 11, 12
The result of this mess is that people cannot update their sites easily, especially if another module they depend on hasn't updated their dependencies other than core.
Better:
layout_builder_restrictions 8.x-2.20 -> Drupal 9.3, 10 LAST RELEASE for DRUPAL 9
layout_builder_restrictions 8.x-2.21 -> Drupal 10, 11
layout_builder_restrictions 3.0.0 -> Drupal 10, 11 (NEW API CHANGES)
layout_builder_restrictions 3.1.0 -> Drupal 11, 12
If I have a Drupal 9.3 site with layout_builder_restrictions 8.x-2.20 installed, this is the last version of layout_builder_restrictions I can use on this site until I upgrade to Drupal 10 or 11. The fact that I removed the Drupal 9 BC code in layout_builder_restrictions 8.x-2.21 is not a problem because it will not install on a Drupal 9 site.
You as a developer don't have to support that old usage, but you do need to signal to your users that you intend to break that old usage. As a site owner I expect if version 3.0.0 of a module works on Drupal 9.5 than version 3.99.1 should also work (even if D9.5 is EOL and I haven't used it on my site in 5 years, I should be able to fire it up and load 3.99.1 and it works.)
Disagree. I expect that composer will install the most recent version of a module I specify that is compatible with my site. I also expect that the version that gets installed will work with my site... meaning that if the module removes support in code for my version of Drupal, it also updated composer to ensure that version will not install on my site either.
If we follow this logic, then what about contrib dependencies? Should I make new major versions of my module when an upstream dependency makes a new major version? If geofield makes a new major version, how do I know if the upstream major version is an API change vs a Drupal core change? Would geofield_map need a new major version? You can see how this cascades into a mess of new major versions that becomes untenable. This is why contrib major version releases should be reserved for its own API/product major changes rather than what its dependent on.
You can and should drop previous Drupal versions in minor releases
Unlike code and API changes, upstream dependencies are managed by composer. Thus, removing BC code and upping the minimum requirements will not cause breaking code. Why? because the new version will not install on unsupported sites!
Unless you plan to actively support and make releases for the no longer supported version of Drupal, there is no point in making a new branch and subsequent release. Even if you do plan on making future releases, you could do it with minor releases (1.1.x support 9.5, 10, 11 vs 1.2.x supporting 10, 11)
Same with PHP
PHP version support should follow core's requirements. If your module supports Drupal 9.5, it needs to support PHP 7.4. Its fine to introduce support for newer versions of PHP as long as it doesn't break the minimum requirements. Dropping php support should follow core: Once you support Drupal 10 as a minimum, PHP 8.1 is minimum. Nothing changes here in regards to major/minor version changes; removing Drupal 9.5 from composer means no Drupal 9 site will get your new version, despite it being only a minor release.
Why is this important?
Modules do not exist in a vacuum. They live in this larger ecosystem with sites and other modules dependent on their code to work. When core and modules make new major versions, it fractures our ecosystem → . As contrib maintainers, we should do everything we can to minimize the dependency pathways by adding BC shims, supporting core versions of Drupal at least until EOL (if not slightly past), and when we do drop support, rely on composers dependency system and not making new major versions unless the module's API/architecture is changing.
Looks like a good feature. would like a test for it though.
japerry → made their first commit to this issue’s fork.
Looks good! Committed!
Thanks ericgsmith for the MR! I've been playing around with the test failures as well to no avail. Going to merge in your MR for now but keep this issue open and NW due to the remaining failures...
Hmm I think a more elegant option here would be to throw an event, allowing the max_age variable to be changed. What if other modules have opinions here?
Marking as a duplicate of 📌 Clean up services, add autowiring, make use of new features, and fix deprecations Active so we can work there. These changes are superficial and won't make Purge d11 compatible.
Cannot replicate either. I'll let it sit here for a bit to see if anyone else sees it, but guessing its a fluke...
Huh I cannot seem to find any evidence of why that was added with the drush command refactoring. Definitely shouldn't be there, sooo removed!
japerry → made their first commit to this issue’s fork.
As these are critical names for site controlled settings, this will not change in a minor version release. Right now, there is no plan for a new major version release of purge, and this change alone isn't enough to justify a new major version and the downstream implications that come with it. However, if a new major version of purge is planned, renaming these variables will be part of that plan.
Thanks for looking at this, as it will be required for D11 compatibility. I've added this issue to our internal D11 tracker as well.
2.0.5 is out now, which hopefully resolves this for everyone!
Yah, if there are patches, please put them in new issues here. Since I know Kirsten, I've added her as a co-maintainer.
Yup! MR7931 works, tested with both Acquia Connector and Search API Solr. I think thats a good workaround, and should throw the deprecation notice as we're updating to Drupal 11.
Its been 4 years since I revisited the event dispatcher deprecations, but I vaguely remember the need to directly call ContainerAwareEventDispatcher because it would inherit the correct EventDispatcherInterface depending on what version of core (8.9, 9.0, 9.3, etc) you were using.
At this point, I think its safe to say we no longer need (or should) support Drupal 8 or early version of 9 (9.5 would be nice since 40% of sites are still on it), meaning any module using this workaround can update their calls to use Symfony\Contracts\EventDispatcher\EventDispatcherInterface.
However, with 10.3 only a month away, and the real likelihood of modules not being updated before this, sites will end up randomly breaking live. So instead of changing the service definition, is there some way to throw a deprecated notice if you're calling the class directly from your code? The Change Record → should also be updated to tell maintainers they need to update their site if they used it as a type hint for the event dispatcher.
If anything, maybe move the change to 10.4, allowing modules more than a month to update?
japerry → created an issue.
Also backported to 1.0.x
I don't know if there are other media type modules that have remote entities, but this issue fundamentally breaks that premise by requiring a media id. See https://www.drupal.org/project/acquia_dam/issues/3442662 🐛 Incompatible with Drupal 10.3 and higher Needs work
For Acquia DAM we're working on a fix, but while its believed this change is internal, I 'think' (??) its expected modules can extend the view to include their media entities. Regardless, I'm leaving a comment here on the chance someone will run into this when updating to 10.3. It'd be nice if there was a BC shim in 10.3 so existing media modules could work.