ModuleHandler::addModule() and ModuleHandler::addProfile() require that the module and path point to a valid module and if they are invalid weird things could happen. Sun said these should use the @internal annotations(#340723-185: Make modules and installation profiles only require .info.yml files) and these methods are for special cases like the installer. Even so, since they are public we should clarify. This clarification will also help regardless of accepting use of the @internal tag.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 2244681-invoke-hooks-on-enabled-modules-9.patch | 9.47 KB | neclimdul |
Comments
Comment #1
neclimdulDope, copy and paste error.
Comment #2
sunHm. I think we should rather throw an exception.
Comment #4
neclimdulyay! we found bugs!
I didn't fix any of the failures but here's a unit test of the functionality.
Comment #5
sunHopefully we can agree on this one...
Comment #6
sunRemoved bogus preloaded module definition.
Comment #7
sunConstructor-inject document root directory (defaulting to DRUPAL_ROOT) for test purposes.
Comment #8
sunSupply DRUPAL_ROOT through a new getAppRoot() method instead. → Clean unit test override.
Comment #9
neclimdulI prefer this approach.
Bogus preload is... truth is this test was pulled out of a larger patch to fix this specific issue. We could remove it but it provides a more realistic module handler for the error tests and its used in other tests I wrote so I'd prefer we keep it.
From discussion on IRC, my opinion:
Mocking module handler limits scope of the test to the code we're testing. Its not different than any of the mocks we build in our simpletest tests its just using the phpunit's mockBuilder to do it. resetImplementations() is a shared internal method so rather then test it on each public method that calls it, I chose to test that separately for clear units of testing and that's my design decision.
DRUPAL_ROOT is accepted global state and is expected to point to the root of the Drupal code base, lets not tackle that in this issue. I propose a follow up to discuss. This "pollutes" our tests with a global but one that is currently fundamental to Drupal, probably more so then the constant we're defining already in bootstrap.php.
Comment #10
neclimdulum... my patch...
Comment #12
sunconflict.module--
Comment #13
sunre: #9:
The unit test will most likely need two separate
ModuleHandlerinstances in the future — one with a constructor-injected list/set of extensions, and another one that is empty.While I can see the argument of testing a single method only, I disagree with mocking the class code under test through a dynamic mock code generator. That exceeds the level of sensible mocking in my book, because it's entirely not clear what the final state of the dynamically generated code is - unless you happen to know PHPUnit's internal code inside out.
I doubt that anyone seriously understands those internals to fully understand the consequences. That's why I'm strongly opposed to the approach of mocking the class under test.
The DRUPAL_ROOT problem space becomes a problem anyway, as soon as other
ModuleHandlermethods are being tested. The solution in #8 (by @msonnabaum) is finally one that makes sense to me (as a pattern + general approach for unit-testing classes that rely on DRUPAL_ROOT).Comment #15
neclimdul1) Having written more tests on this class, maybe in the future but not for any of the ones I wrote. If that's the only thing blocking this issue I'll take it out though.
2) So first, the test works, you can muck up the implementation and see this. Second, we don't need to understand phpunit's code base and I don't feel like I should have to validate that its code works to use part of its API. I don't know every line of code making it work but the code is extensively tested not just in their tests but by being very widely used. That's why we're using phpunit.
For reference, the relevant documentation from the phpunit doubles section.
Below this example you'll see the documentation.
First bullet:
Then lower the documentation for setMethods():
3) I stick by my argument with respect to this issue that this is a larger problem. Let me be a bit more clear, we should not make a special snowflake of this class. If we're going to fix this lets discuss it and fix it as a community not try to sneak something in an unrelated issue for this random class without a larger discussion. That may very well be the correct fix, lets not fix it _HERE_.
Comment #16
sunApologies, I was wrong here. Mocking the class under test in order to only test a specific unit (method) is indeed a possible testing practice. I have a better understanding of PHPUnit's internals now.
So unless this test has been committed as part of another issue already (IIRC, I've seen some similar code elsewhere?), let's move forward with your patch, @neclimdul. :)
Comment #19
marvil07 commentedIndeed several hunks here are already inside core.
Moving to NW, since the patch does not apply anymore.
Also, based on last sun comment, I'm hiding not relevant patches.
Comment #20
marvil07 commentedComment #28
avpadernoComment #32
nicxvan commentedThis can be closed one the related issue gets in.
Comment #33
nicxvan commentedIt's in, I think we can close this, just not sure on credit.
Comment #35
nicxvan commentedThe referenced methods have been removed.