Added the disabled deprecations variable until #3400979: Is SYMFONY_DEPRECATIONS_HELPER: weak correct for contrib deprecation testing needs? → is resolved as a temporary workaround. Merging this.
@vishalkhode: Please do not include screenshots of executed tests (just like you should include screenshots of applying patches), that's considered trying to game the credits system and a waste of disk space on drupal.org
I think the error there happens when there are no facets, and it's still converted into a checkboxes or similar element without any options.
D11 tests are not passing yet.
The previous major was there on purpose, see #3447089: Disable previous major testing with D9 so that modules can prepare for D11.0 → but it looks like it isn't going to happen like I'd prefer so, so maybe I'll leave it out here for now.
I also just had a case in a project where the current behavior was unexpected. #20 wouldn't work for me, because it wasn't a direct link, but part of the trait. In my case, the deeper menu link wasn't actually want I wanted, it was the one higher up that was in a completely different section of the site. The deeper one was actually a cross-reference from a different section of the website.
So what would work for me is just removing the array_reverse(), which basically makes the behavior consistent with core. But that seems rather arbitrary either way. In my case, the deeper link also had extra query arguments. The logic in #19 and rerolls also seems rather arbitrary, which might or might not be what you expect.
There's ✨ Allow other modules to alter the active link trail Needs work as well, if you want to apply extra rules on matching. For this to be committed in a BC way, the behavior would probably need to be configurable and especially if it gets as complex as latest patches, requires pretty extensive test coverage to cover all those extra checks.
I think it would make more sense if that's implemented in the frontend *before* saving, otherwise you have to go back after you already saved and edit it again. Similar to the you have unsaved changes message after reordering.
That said, opinionated things are tricky to do in core. first, it should be in menu_ui, because node module should not depend on menu link stuff, then it needs to handle cases when the node title field isn't displayed and so on.
As a maintainer of many contrib projects, I very much value that all those versions are now managed as a recommended default. I always found it annoying that you had to select the right combination from a very long list of versions and the majority in there weren't even supported anymore.
> So you could have for now OPT_IN_TEST_NEXT_MAJOR: 1, and then once we do the above swap, change it to OPT_IN_TEST_PREVIOUS_MAJOR: 1. That way you'll get Drupal 10 and 11 before and after.
Yes, that's exactly what I'm trying to avoid, having to do that.
The remote include is an interesting workaround, it does feel weird to have that configuration outside of the repository and even outside of drupal.org. It also means nobody but me is able to make a change to that. Sure, they can still override it either directly in the file or or point to a different locations, but then it's theirs.
I honestly don't understand the reluctance to stop D9 testing (again, I'm not suggesting to switch to D10, just switching to disabling it, you will soon need the ability to not have next major or previous minor testing anyway). For Drupal CI you (DA/DrupalCI maintainers) were pretty quick to force disable all Drupal 8 and 9 testing configurations when they were no longer supported. But that's your call I guess.
Left some comments. This should opt in to NEXT_MAJOR testing so we can verify the tests actually work on D11.
Berdir → created an issue.
No idea what that line does, this was ported from D7 code that was quite different. seems fine to just drop this, yes.
Berdir → made their first commit to this issue’s fork.
[node:summary] is provided by the core node module, not token.module, and it essentially follows the same logic as the summary or trimmed formatter. If that behaviour is not what you want, then I would suggest you implement your own token. Until the text with summary field type is deprecated/removed from core, it's unlikely that this behavior will chnage.
I think I clicked on the wrong MR link, was on my phone, sorry :)
Changes look good, I manually confirmed that this fixes the test fail I had in my project.
Also confirmed that the code flow in UserAuthenticationController wasn't changed and should also be OK. That said, I think some of the flood related optimizations were apparently not applied here, flood control still does an extra username-based lookup here, which also means that it doesn't support flood control for an account returned by \Drupal\user\UserAuthenticationInterface::lookupAccount(). But that's not a regression and can be looked at in another issue, possibly the one that does the work to unify user login flood control.
This is not yet changing UserAuth to not implement the new interface nor is it deprecating it. And it's not yet updating the UserLoginForm (and UserAuthController I guess).
Merged. Will need an issue to add a require-dev for book module for D11, doesn't work yet.
> What path forward do you propose?
I don't know. The commerce issue has been fixed and will be available in a release soon. It's now in the alpha release, so I guess reverting this would just cause a bigger confusion. Might hit people trying out the alpha release, but nothing can be done about that at this point I guess except getting out a new commerce release out asap.
The combined usage with the strict type type is special, but using that config is not.
From your comment in the other issue:
> IOW: this is all caused by this module using core config as if it was this module's config.
That's not a thing. Drupal has the concept of a default country, and reading this config file is the only way to access it. There is no rule that you're not allowed to read config of other modules.
Using config like that is common. See https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22get%2...
https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → is IMHO a bit confusing, as it then only lists composer related config, but I understand that as changing/removing config structure is covered by our BC policy.
@jsacksick? Yes, that's exactly what the MR does? It additionally also checks for it to be a string to be extra careful, just in case that config is something else, but an empty string will skip the check and return NULL
That's what the code kinda does, I think the is_string() call is not necessary. The difference is just that an empty string kinda worked until now, while a null value doesn't work at all.
At least the local resolver, which uses the country resolver checks if there really is a return value.
Because it was optional before as well, the only change is the move from empty string which was bogus but not broken, to null.
The relevant core issue is actually 📌 Add validation constraints to system.date Fixed
This breaks commerce hard, because it expects that country is a string, so this change results in a fatal error there atm. See 🐛 TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given RTBC . While it can easily be fixed there and the change makes sense anyway, instead of returning a Country object with an empty country string, this is still a pretty hard BC break.
It looks like this heavily overlaps with ✨ [2.x] Allow authentication via mail and password over RPC Needs review . The new test passes even when EmailRegistrationServiceProvider is commented out, because that issue does its own mail to name lookup in the controller.
I was looking into this because of 🐛 UserAuth BC layer is not working for modules that use it to provide email based logins Active , because of that other issue, this module is currently _not_ directly affected by the 10.3 changes. (Apart from the failing commerce tests, due to 📌 Remove country setting from the installer Fixed . It will need to eventually adapt to those UserAuth changes in both the UserAuth implementation and the copy pasted controller, but that's a D12 deprecation. And on the plus side, that willl allow to remove a ton of code as only the new lookupAccount() method will need to be implemented.
Berdir → created an issue.
This became more relevant in 10.3, which removes the country setting and the implicit empty string value being set there, resulting in this fatal error with basically all tests on 10.3 right now. See 📌 Remove country setting from the installer Fixed . This might still change, but it doesn't hurt to make this change. It's quite possible that the setting will be completely removed from core in the future.
contrib doesn't fail in deprecations by default, so do we really need that?
I think so.
> #[Hook('hook_form_FORM_ID_alter', form: 'myform')]
I don't think your proposal makes it any easier to use, it's longer, and you still have to remember the pattern, which tends to be confusing as many forms have a "form" in their form_id (is it form_FORM_ID_alter() or FORM_ID_form_alter()...)
What we *could* add is something like a #[FormAlter('form_id')] or #[EntityHook(entity_type: 'node', hook: 'presave')], specific attribute classes that then build the specific hook name patterns that they know about. That could easily be a follow-up, the discovery already explicitly supports any Hook subclass (for example the already implemented AlterHook), here can be as many as we want, you could even add your own.
Contributions to 4.x should use merge requests so that tests can run there.
Turns out that I don't have permission to add new maintainers, but +1. We'll need to figure out who is allowed to do so.
In the meantime, you're welcome to help already by reviewing issues as well as creating merge requests and other contributions. More tests would be amazing, not sure if real integration tests are possible?
Once you are added as a maintainer, try to give other maintainers a chance to review changes and try to avoid committing your own non-trivial changes as much as possible.
Merged.
Looking at the changes again, the previous implementation explicitly supported #title as NULL, I can't really imagine how that would be desirable and there don't seem to be any tests for that? But it would be easier to expand the ?? to an if/else again if we want to keep that.
Pushed a fix, not sure about tests here. This is a pretty severe regression from 10.2, completely breaks a module like moderation_sidebar and the new implementation is quite a bit simpler (and also a bit faster, since it avoids calling getTitle() twice.
Seeing fatal errors caused by the problem mentioned in #135 on 10.3.
@sakiland: The approach to handle such an issue is either to revert the original issue, which would be too complicated at this point or create a follow-up, did that now: 🐛 DialogRenderer::getTitleAsStringable() does not support all return types of TitleResolverInterface::getTitle() Active . Working on a fix for that there.
Berdir → created an issue.
catch also tagged 📌 Replace hook_cron() with a more modern approach Needs work as related. Being able have multiple hooks in a module would definitely solve one aspect that the cron subscriber issue is trying to solve, another is the ability to have cron subscribers in core components. Would this allow to also basically treat each component as a "module"? That would allow to move a lot of the code that's currently in system.module, especially hooks, to the respective component. Similar to how we already support plugin discovery there, each core component could have a Hooks folder as well.
Sounds like that is related to what I was fighting with in #3383487-14: Add CronSubscriberInterface so that services can execute cron tasks directly → as well?
Rebased.
Created a merge request.
Rebased, removed statistics.module changes.
I replied on the follow-up.
There is the NotNull constraint, added by \Drupal\Core\TypedData\TypedDataManager::getDefaultConstraints().
There are a number of tests for this, for example \Drupal\KernelTests\Core\Entity\EntityValidationTest::checkValidation(), or \Drupal\Tests\taxonomy\Kernel\TermValidationTest::testValidation(), or rest/jsonapi tests like \Drupal\Tests\jsonapi\Functional\CommentTest::testPostIndividualDxWithoutCriticalBaseFields.
Maybe something doesn't work correctly for configurable fields, maybe something isn't set up or overrides this? You will have to investigate if you get the constraint added for your field, if it triggers and what it checks.
Merged.
FWIW, one thing to consider is backwards compatibility. If a custom site also defined a bundle class for the forum node type, then either this won't be used as it's overwritten, resulting in fatal errors/non-executed blocks of code if they check for an interface or so. Or the custom class is replaced, equally resulting in either fatal errors or code not being executed.
BC would be quite challenging to handle.
#8: Bundle classes are just a neat way to organize code, they don't allow to do something that can't be done otherwise as well. In this case, a setting to configure which node types could be used in forum. The only difference is where that setting is checked exactly, in node hook or so, or in the bundle info alter hook. In other words, not really related.
#7 sounds like a ChatGPT explanation to me.
There is no need to have a cache bin for every little bin, Drupal 8+ optimized for fewer, semantic cache bins instead of basically having one for every module.
Also, this is just the tree cache, which is only used when displaying token UI, the actual tokens are cached in cache_default and that's defined in core.
I'm not "dead set" against it, I just think it's not worth your time and is unlikely to get committed. This isn't a 10.3 regression, it's already in 10.2.
Yes, plugin looks bad, but it can be improve to look better in both 10.1 and 10.2 fairly easy and again, plugin is an exceptional case.
alexpott → credited Berdir → .
This is failing tests.
Still needs work per #8, dependency version information needs to be updated to require a linkit version that supports this API.
No need to repeat feature request in the issue title.
The recommended way to display media entities is through view modes. Then each media type can have its own formatter configuration and you can mix and match bynder, images, videos and whatever else you manage with media entities.
BynderFormatter does work on the entity reference to the media entity as well, but this is exactly why I didn't recommend using it like that, Create media view modes and render those.
I didn't see this issue when I worked on mine. What I struggled with in my implementation was making the necessary context available to the event in a useful way. Many of the commerce plugins work in different ways, their context differs and the data they produce differs. My MR focuses on unifying that and having more specific events that work for many different events.
I don't understand the change that was done to user_role_revoke_permissions() and user_role_revoke_permissions(). The logic seems inverted git land there is absolutely no reason to ever load roles with overrides in those functions. Config entities that are saved must always be loaded without overrides.
I guess it was done to fix Drupal\Tests\text\Functional\TextFieldTest::testTextfieldWidgetsFormatted() but that doesn't seem right, the test fail I think is valid and points to a flaw somewhere. And I think that flaw is how role config entities are statically cached. \Drupal\Core\Config\Entity\ConfigEntityStorage::buildCacheId() is dynamic based on overrideFree being enabled or not. That means two versions of the same config entity can be cached at the same time, but saving is only going to invalidate one of them.
I've now added an overridden version of that invalidates always using the tag, that means it always invalidates all entities of that type, but I think the overhead of that is minor as config entity saves really shouldn't happen that often.
There's a second change here that on it's own would also fix the test. The specific reason it happens here is that the test disables all text formats, which triggers user_filter_format_disable(), which removes that permission, but this doesn't use loadOverrideFree() yet, so it's invalidating the non-override version. Changing this on its own also fixes this test, but I think it makes sense to fix resetCache() as well.
I expect that those changes might trigger some unit test fails, but we'll just have to update that then.
Also did a first pass of an issue summary.
plugin is a special case, I don't think any other module add more. For most it's 1-3 or so and you really need to rethink the category to fit into the new UI.
Paragraphs is a special case, it's not not a field type, it's a predefined option that is being promoted up, like media. The basic example is this: https://git.drupalcode.org/project/dynamic_entity_reference/-/blob/3.x/d....
We have BC handling, it doesn't break and it adds it to general. I don't think we should spend time in core to do more, because there is no straight forward conversion. It's a completely different UI and most modules that use a category will need to rethink that and instead consider to use an existing category. Plugin could also be seen as a reference, only the fact that there are 20 of them (which probably really shouldn't work like that, it should most likely be a setting, like entity references but there might be a technical reason why that's not possible, I don't remember) is a reason to have its own category.
This is by design, they are not meant to be supported. categories are their own definition now, it's not just a string.
Several modules, including paragraphs and simplenews have examples on how to be compatible with both 10.2 and earlier versions. Plugin needs to be updated too.
See https://www.drupal.org/node/2910500 → . Don't use root user.
Yeah, I didn't add an update function just to clear caches, you should always clear caches after module updates, even or actually specifically if there are no updates.
This code doesn't exist in this module, you are most likely using a patch, one that you probably shouldn't.
And even if it's valid, you shouldn't (and can't) re-open an old closed issue, create a new issue, and reference it.
Merged.
Found the problem, I needed to drop the $legacy_contexts and instead just pass $contexts through. Thanks for catching that!
And yes, that single 200/403 line is literally the whole test coverage that exists in core for the current entity_upcast behaviour, and we only added that in a separate issue as part of the translate-editable-content permission. There's that unit test mock, but that does not test what's going on inside EntityRepository.
Thanks, would have been happy too without the change record, but won't say no to it now that you created it ;)
Rebased. Rebasing this is pretty tough with the formatting commits, we might want to squash that all together. And just get it in asap.
Addressed my review. Also deprecated the constant, FWIW, there's not a single usage of this in all of contrib.
Gave feedback in slack too. You need at least 10000 anyway right now if you want to to match your version number as the minimum is 8000. The only difference is that extra 100 you have in there, and if you really don't want that, you can add another 0 to make it 100000.
100x update numbers are not supported.
Update version number don't care about core/contrib, semver or not. It' just a number and any new number needs to be larger than the previous one and it needs to be larger than 8000 until this issue is resolved.
I agree that it would be useful to rename and lower the constant from the historic 8000 to anything else, but it's not as trivial as it sounds (any existing installed module has 8000, so we'd need an update function to lower that) and it will not happen in time for you.
I think this is a duplicate of 🐛 Error on hook_install when invoked from drush site-install Fixed at this point. The user module check is bogus as user is a required module, but it uses the API function that is patched to use loadOverrideFree() in the related core issue.
null needs to be uppercase.
Agreed, happened to me too.
But just in case there are tests like that out there, lets keep and @deprecate the old class, that should also make it unlikely to be picked up by phpstorm like that.
There's no need for such a long comment, and it should not use get(), it should use getType(), which is a method on FieldDefinitionInterface.
Committed.
Made that change. There's not a lot in tests and those things can be fixed.
Not sure what to do with this.
Each of those MR's will conflict as they all change the phpcs and gitlab config.
I'd rather not change those files, including the allow fail. I'd rather rely on the code quality difference, which gitlabCI I think is still working on properly supporting. Then we can see if the situation gets worse or improves in a MR.
drupal.org issues are also not well suited to merge multiple merge requests per issue. We should either split them into separate issues or combine it.
Merged, thank you.
Merging.
Thanks for the review. Fixed the check and removed the static cache.
Converted to an MR and rebased, this conflicted pretty heavily with a change that converted to short array syntax.
I slightly changed some of the ternary conditions, so that the fallback is an empty array and not an array with NULL.
The method signature was not changed in the core issue, so updated this and converted to a MR.
Early testing shows that this saves about 5-6MB of memory on a site with a bunch but not huge amount of fields, I've seen token info caches that are a lot bigger.
We found another use case when deleting a config entity causes other entities to be updated, so we need those to be loaded without overrides as well.
This would need to consider cacheability metadata for http headers, which I'm not sure is reliably possible as page cache for example won't rely on that.
Ajax should maybe have its own handling, similar to what views already does in its ajax controller.
There are some HEAD test fails that have been fixed and also some known random fails ( 📌 [random test failures] Use key/value directly instead of state in test modules Active but I also can't reproduce the migrate fails locally.
You can download the artifact of e.g. https://git.drupalcode.org/issue/drupal-3421014/-/jobs/1261546 and look at the browser output, but I'm not seeing anything obvious from that from a quick look.
I think the logic for writing the services.yml file needs to be in \Drupal\Core\Test\FunctionalTestSetupTrait::prepareSettings, that's why my proposal is to deprecate the trait entirely and just have the flag there.
Left some comments on hopefully pragmatic solutions for these cases.
If that doesn't work out somehow then +1 to skip problematic ones in this first pass and deal with them in a separate and smaller issue, so that we can unblock the deprecation of plugin managers that don't support attributes.