Looks right!
I did my normal check except the zebra check.
All deprecations are right
No double __
They all set __FUNCTION__
All initial preprocess are for the correct method.
All comments are correct.
I'll do the zebra check later before marking this.
nicxvan → changed the visibility of the branch 3544715-convert-theme-hooks-consolidated2 to hidden.
@longwave I think more changes are needed if we take that direction, the removed getThemeEngine method is used by Registry which is causing the failures afaict.
I reverted some of the consolidation so the tests should be passing while people review the high level concepts, I'll create a separate consolidation branch to work on consolidating some of the kernel duplication in the meantime.
nicxvan → changed the visibility of the branch 3544715-convert-theme-hooks-consolidated to hidden.
I broke something while consolidating.
There is more consolidation to do so back to needs work.
We really just need to trigger a batch that alphabetically sorts the parameters and successfully finishes with the patch and fails without.
Honestly if we use the event subscriber we should rename it to alpha sort and remove the company's name.
Ok I did my review too:
No double _
All deprecations for the functions are __FUNCTION__
Versions are all correct
The fixings call the correct method
I haven't done the zebra check yet but that probably won't work on this one.
Two comments on the MR
And the change records mentioned in slack are needed.
Drupal batch processing breaks when you use the query string sort feature in cloudflare.
Batch should not depend on the order of the parameters, just the existence.
The event subscriber is to simulate cloudflare.
We probably could do it differently but batch testing is painful.
However the test only isn't failing so test coverage isn't sufficient.
I converted the deprecation thanks for catching that!
I also rebased and did it manually to add the use statement.
I also updated the CR, it's pretty niche and straightforward. I'm on the fence on whether we want a CR at all for this anyway.
It's ready for review again.
honestly even help is pretty theoretical, most sites use standard and in my experience leave it installed, this could be a reason to start uninstalling it on most sites though.
Thank you for working on this, we know one consequence of this is that parent modules cannot implement these hooks on behalf of the sub modules anymore. It's the inverse of this issue, but the rarer by far most likely. We also probably want to deprecate on behalf of moving forward anyway.
The only question is do we want to do a CR for this since this can break implementations?
I think the bot is wrong, I literally just rebased and the UI doesn't show any conflicts. I think it ran on the previous botched rebase.
Since this deprecates a hook, I'd love opinions on an issue to change how we do that: 📌 Deprecate ModuleHandlerInterface::*Deprecated() in favor of hook metadata Needs work
I added a todo with a link to a postponed issue for the secure deprecation removal.
I think this is RTBC, but will leave it for a bit for other's to chime in.
All feedback has been addressed, I think this is a great step!
I think this is ready for review, let's move the preprocess work to a follow up, I'll continue in the hidden branch here, but this already is doing a lot so we can review it as is.
I know @berdir wants to review options for how we're storing and collecting them, but I think the rest of this is ready for review and discussion!
nicxvan → changed the visibility of the branch 3544715-preprocess to hidden.
I think this is due to no code changes and just documents.
I think this is ready!
Normally you can't RTBC when you work on it without peer review, but my contribution here beyond advice was just a rebase and this is a super simple change I think it's fine to RTBC here.
Let's see if the bot complains about this. The tests should run now.
I'm ok with that since it was the original plan, let me ping catch since he decided not to delete.
I will help here is how I fixed it
Ensure my local 11.x is up to date
git checkout 11.x
git fetch
git rebaseGet this fork
git remote add drupal-3544005 git@git.drupal.org:issue/drupal-3544005.git
git fetch drupal-3544005
git checkout -b '3544005-hookfieldtypecategoryinfoalter' --track drupal-3544005/'3544005-hookfieldtypecategoryinfoalter'
Update this fork
git rebase 11.x
Run phpcs on the file
composer phpcs core/modules/field/field.api.php
Safely push with lease
git push drupal-3544005 --force-with-lease
Spent some more time looking at this.
@catch said it's fine for now to just use the service call directly and then create a follow up which I did.
We should be green.
This is mostly ready for review, I still need to write tests for preprocess, I think I'll pull those changes into the main MR now though.
I need to review the one or two remaining @todo
Ok I added some tests for normal hooks.
Preprocess needs tests and likely some work.
The last failing test messenger trait seems to be holding onto the old messenger service hard.
I even called setmessenger from the theme installer and the confirm experimental form still has the old service.
When in the form calling drupal service messenger has the correct messenger instance and the test passes.
Why does that file permission change? I have a recollection of this happening for me too but always just one file.
Ah I already had a comment on the mr about that, I didn't see that in the bot file thanks.
Not sure what the bot is complaining about. I think we do rename the attribute.
We can always add the skip conditions setting later.
Figured out @berdirs concerns, I left more breadcrumbs in the MR feel free to ping me on slack or here for clarification.
One tricky thing is only the owner or full maintainer can change this AFAIK.
There are several modules once applied that I cannot change this for because I don't have the permissions.
If we could allow maintainers to change the default that might be nice too.
Might be problematic though.
I've updated the credit to add elc.
I think since this had a follow up in drupalorg we might be able to close this now.
Thanks! I added #3547357: Add redirect for two obsolete change records. →
I think the CRs might need deletion still so the redirects can be set up.
Even unpublished they are accessible.
I tried updating the target branch, but the source is also wrong.
Got some comments on the mr.
One question is there one we want to deprecate here or is that out of scope?
Got some comments now on the MR.
One question I have is there a plugin we want to deprecate now?
I created a start for the CR as well.
If we decide to deprecate in a follow up we can move it there.
I think we should do the full deprecation there since I think it's not much more, it's just the preprocess collection and the .engine discovery.
No sorry!
I was just commenting based on #2943436-35: Review all tests where system module is the only dependency → that the other issue was scoped to singletons, I think this is great cleanup and I agree with 7, any reduction we can do is a win.
I'm not sure the best way to actually manually test this, if you have ideas then I can attempt that.
I reviewed the code and idea and generally it seems good, but I have two concerns here based on some assumption in the comments.
We are doing direct queries that are not parameterized.
There is a comment saying that because we are casting them in the argument that is good enough, but I'm not sure for two separate reasons, one I think we can address with a test and the other we need to mitigate.
1. Someone for some other reason could update the int into something else. I think we need a test sending an invalid value and / or a test that does reflection or something on the abstract class verifying that this method requires an int for that parameter. In my mind for these kinds of things a failing test is caught quicker than a change like that cause someone has to see where the parameter is used and most people assume any queries are protected by parameterization. I think extra effort to ensure our safeguard remains in place here is worthwhile.
2. Our assumption that the cast is enough is technically wrong since in postgres we multiply by 1000, if the int is large enough that multiplying would cause overflow then it's no longer an int when passed. Presumably it's then a float which probably just blows up, but this possibility concerns me, maybe we cast to an int again before passing it in, or we ensure the value is lower than some sane maximum.
Honestly I think the first point is more important just as a safeguard, I expect the second issue just results in a fatal since you're passing a float where an int is expected, but there are people better equipped to truly evaluate this bit.
If I'm wrong about point 1 or 2 please let me know.
See https://3v4l.org/31a3W, using a named parameter that doesn't exist is a fatal error.
Thank you for confirming my recollections were wrong, in that case the way you did it is correct.
My concern with the loops btw is what happened with the original ordering approach that it's just an extra loop you have to keep in your mental model.
I'm also not a fan of having these on classes though since hooks can be there we need to support that, I kind of wish we didn't add that in the original hook, but that is very much just personal preference cause then the method would always be defined and tightly coupled.
Let me review this again now that you've validated the approach is correct again.
The other issue was only about tests that has only the one dependency.
I think most feedback has been addressed and this is almost ready.
There are however a couple of things:
1 I think berdir needs to confirm you addressed his concern. I think you did, but his concern was pretty sweeping. Maybe ping @berdir and t@hejimbirch in slack.
2 you mentioned a request to split out the create in response to @larowlan but I can't find the comment you're talking about.
I'm going to officially close this as won't fix due to the risks mentioned in 58.
With config management and drush uninstall many modules even on multi sites isn't much of a burden any more I don't think and the risk is far too great.
I did add credit here for the effort over the years!
Rebased in the ui I'll keep an eye on tests.
Credit updated too.
I think it's time to close this officially.
@xmacinfo if you create that export issue please link it here thanks!
I'm going to close this as works as designed for the following reasons.
Honestly reading through the most recent stack exchange in 43 I think this form actually qualifies and is eligible for auto focus.
99% of the time I visit that page the first thing I do is click in the search field.
The other 1% I ctrl f to search.
However, as pointed out this is a very simple contrib or custom module with a form alter and library so it should be handled in contrib instead of being hashed out here with the ability to disable it.
Then it can be included in cms.
If someone ends up creating that please post it here so people can find it.
I don't think will get this in core though so let's close this for now.
Thanks everyone!
I'm going to close this as I don't think we really have a good place for this that wouldn't already have the type info.
If we think this is worth doing feel free to reopen!
I'm going to close this since the various hook requirements should cover this need.
I think this is a good cleanup.
If someone wants a follow up we can create that, but shouldn't block this further.
To be clear the only one I think that would be good is file, but the filesystem is pretty intertwined with system.
Thank you for your patience!
I think this is ready.
I'll create a follow up for the ckeditor5 cleanup in a bit.
This is so good!
Let's get the summary updated, the CR started etc.
I think the function deprecations linking here is ok.
I have one question I'm not sure about how it works.
A couple architectural questions we need to resolve.
A few minor clarifications.
We might need to search documentation to update how theme engines are created too.
Thank you, this one has been on my mind.
Now that I see it I have a pretty strong feeling we shouldn't do it this way.
The extra loop is not great and we're again affecting implementations from a second location that needs a loop. We ran into this with the first pass of the ordering issue if you recall.
I think we should look at adding a parameter since it will just be an array I don't think will hit the same bc wall we did with ordering since there is no object. (We need to verify)
That keeps the condition local to the implementor.
Which brings me to the second point:
I wonder if it's worth it to special cases a few hooks like this one and optimize them out by default
Initially I loved this line of thought, but can we reliably do that? What if someone else incomes help? They won't be available.
If we're OK with that then it's a nice optimization, but I'm concerned it works break things.
Maybe we add a setting to ignore conditions in both cases? Then people that set it can call remove hook on the implementations they still want gone?
I made suggestions for the remaining dependencies if you can apply them through the gitlab interface.
Sorry, what I meant was the test module dependencies.
So for example you left: testing_config_overrides_module instead of drupal:testing_config_overrides_module
What I was looking for was an example of a test module that had a dependency on another test module and what they did.
Here is an example: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/profiles/test...
That means we should use drupal: before all dependencies.
I'll create some suggestions in a bit, no need to do this in a follow up.
We should do a follow up for cleaning up other dependencies. E.g. a bunch of ckeditor test modules use ckeditor5:ckeditor5
Ok I got the theme and translation tests.
Only remaining test is:
Experimental Theme (Drupal\Tests\system\Functional\Theme\ExperimentalTheme)
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...
Experimental confirm form
┐
├ Behat\Mink\Exception\ResponseTextException: The text "The Experimental test theme has been installed." was not found anywhere in the text of the current page.
It is reproducible in the UI.
When enabling experimental_theme_test you get the confirmation to enable an experimental theme but no message once it is done.
The code does hit this line $this->messenger()->addStatus($this->t('The %theme theme has been installed.', ['%theme' => $themes[$theme]->info['name']])); in ThemeExperimentalConfirmForm.
I think this is technically supposed to be needs review.
What is this fork from? Those are not core tests and it shows being 45000 commits behind.
Re 14 yes tests won't run in MR between branches without a lot of work.
It's still helpful sometimes to see the diff!
Adding the plugin clear and router rebuild fixed most of the remaining.
Failures:
Twig Trans (Drupal\Tests\system\Functional\Theme\TwigTrans)
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...
Theme (Drupal\Tests\system\Functional\System\Theme)
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...
Experimental Theme (Drupal\Tests\system\Functional\Theme\ExperimentalTheme)
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...
I also started working on preprocess.
nicxvan → changed the visibility of the branch 3544715-themeproceduralcall to hidden.
Knocking them down!
Just a few failures left:
Config Translation Ui Theme (Drupal\Tests\config_translation\Functional\ConfigTranslationUiTheme)
Twig Trans (Drupal\Tests\system\Functional\Theme\TwigTrans)
Breadcrumb (Drupal\Tests\system\Functional\Menu\Breadcrumb)
Theme (Drupal\Tests\system\Functional\System\Theme)
Experimental Theme (Drupal\Tests\system\Functional\Theme\ExperimentalTheme)
For the first the route provided is access denied.
For the last the install message doesn't display
Haven't dug into the others.
Looking at the Functional Javascript failures:
One is MediaLinkabilityTest.php
$assert_session->assertNoElementAfterWait('css', 'figure.drupal-media > figcaption');
Not sure why that's failing yet.
The other is CKEditor5Test.php
This is just a new order of the attributes but they are all correct. I think we can just update the order in the test.
I fixed Drupal\Tests\block\Kernel\NewDefaultThemeBlocksTest by just getting a fresh installer like you suggested.
Then for Drupal\Tests\block\Kernel\ConfigActionsTest i used the block test theme which also had a content region.
I'm starting to work through Functional Tests, the first is that messages are not being displayed relating to install which i suspect is a similar issue, but if i recall correctly container build persists messages.
nicxvan → changed the visibility of the branch 3544715-kernelTestBase to hidden.
Thank you so much!
I'm wondering if we should reset some of these somewhere or just let these be test only artifacts.
I ready the steam wrapper in theme install and that makes Drupal\Tests\editor\Kernel\EditorValidationTest pass.
Thank you for validating my umami weirdness findings, I had thought it was a red herring, but you found what I found, it initially calls the scam with no profile at so it finds everything, then when it gets to the test the profile is set to a test one so umami isn't found.
Let's just change to another theme for the test.
I would usually ask in slack in this case.
Usually in my experience it means the namespace is wrong. I don't think that is it in this case though.
I know @group has been replaced with the attribute, maybe @covers was too?
We do need to update @group to the attribute though.
We gotta get that MR green before we fully review. There are still phpstan failures.