Closed (outdated)
Project:
Drupal core
Version:
11.x-dev
Component:
theme system
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
4 Apr 2014 at 13:06 UTC
Updated:
14 Jul 2025 at 01:16 UTC
Jump to comment: Most recent
ThemeTestSubscriber
Consistently using ContainerAwareTrait in core has unveiled 3 cascaded bugs hiding each other from detection see
https://drupal.org/comment/8643347#comment-8643347 (2)
The relevant extract :-
(1) ThemeTestSubscriber extends ContainerAware it does not need a service_container it needs an IntrospectableContainer.
(2) In any event theme_test.services.yml does not as expected inject a container of any form.
(3) The place where the uninitialised container is used is "unreached code" by the testbot hence no bug is picked up!
That issue, is limited in scope, and so has just left the stub of a fix as the @todo in the ThemeTestSubscriber.
This issue is to ensure
1) The mocked container is initialised
2) The unreached code is actually used.
Comments
Comment #1
star-szrSince this is just changing tests it could be committed during RC if we get a patch here.
Comment #4
joelpittet@martin107 could you post a starter patch from your interdiff in the other issue?
Comment #5
martin107 commentedI will take a look ... it was so long ago that I posted this .. sorry it slipped off my todo list.
From a cursory glance I see IntrospectableContainers are now deprecated -- so the solution may differ....
IS change.
I could not spell cascaded.
Comment #6
joelpittet@martin107 Thanks, I know the feeling, just cleaning the theme queue and spotting the ones like this. Improvements to the tests should be ok get in 8.3.x for sure and if this is a bug 8.2.x also has a shot.
Comment #7
martin107 commentedAh thanks for prodding me ...
A few comments
ThemeTestSubscriber does not appear in on the webtest interface ( /admin/config/development/testing )
It doesn't appear on the test output generated by testbot ( for example https://www.drupal.org/pift-ci-job/477903 )
The service named 'theme_test.subscriber' that uses the class in its definition but is never ever used!
Running the test from the console
sudo php \
./core/scripts/run-tests.sh --color \
--die-on-fail \
--url http://dev.drupal.co.uk \
--verbose \
--class 'Drupal\theme_test\EventSubscriber\ThemeTestSubscriber'
where dev.drupa.co.uk is the name of my dummy installation - show that the construct() definition has a unrelated flaw.
So the class as a whole looks broken or is redundant.
I am afraid I am not too familiar with the theme test strategy which looks extensive.
Is the reason for the test still relevant ? .. if so I want to change this issue to resurrecting a wanted zombified test?
or can the code and service definition simply be deleted because other tests provide the necessary cover?
Comment #8
mile23@joelpittet asked me to look, so:
Of course it doesn't run. It's not a test. :-) It's a fixture. It first shows up in #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 though it looks like that's a giant patch that didn't mark the file as moved, and instead created it. No doubt it has a real history preceding that.
It's part of the
theme_testmodule, which is installed for tests likeEngineNyanCatTest. Yes, that's a real core test. :-)It looks like the event listener part is used by
ThemeEarlyInitializationTest, and maybe others.Marking this as postponed because I'm not sure whether there's still a bug or whether this explanation makes it go away. @martin107 can you check it out and see if it's still relevant?
Comment #9
martin107 commentedEngineNyanCatTest and ThemeEarlyInitializationTest
Ah that is really valuable information... it gets me over a hump in analysis.
Mile23++
That looks more possible :)
As far as I can see as the fixture/module is enabled ThemeTestSubscriber::container is still used in the class but not set.
So it is A or B
A) Functionality has been reduced. and we need to restore it.
B) The fixture can be slimmed down to only things that are useful to testing core code.
I will find time next week to look at this some more.
Comment #18
catch#2721315: ThemeTestSubscriber::onView() doesn't do what it claims was duplicate.
Comment #22
acbramley commentedSo this code was introduced in #1886448: Rewrite the theme registry into a proper service, swapped from _theme_registry_callback and _theme_test_load_registry which just did a print.
I grepped for the string "registry initialized" which is checked in \Drupal\Tests\system\Functional\Theme\FastTest
Debugging that test, ThemeTestSubscriber::onView is never hit. This is because MainContentViewSubscriber::onViewRenderArray runs before it and calls setResponse which stops propagation.
Increasing the priority of ::onView to 10 will cause it to run BUT exposes even more issues with the test.
1. the test hits user/autocomplete which is not an autocomplete path, it's a user path. The test hits a 404 page
2. The test checks the response contains the account name, which it does, but not in any markup.
3. The test checks for the absence of the "registry initialized" text, which I don't think would even happen if everything was working as expected because the event subscriber throws an exception, it doesn't print anything.
Comment #23
mstrelan commentedComment #24
berdirCan we close this now that the referenced issue is in?
Comment #25
acbramley commentedAbsolutely, added credits for people who worked on this.