Problem
-
Various code spots throughout core are validating the length of extension names, whereas...
-
An extension with a too long name MUST NOT enter the application in the first place.
Proposed solution
-
Centralize extension name length validation into
ExtensionDiscovery
.
Comment | File | Size | Author |
---|---|---|---|
#24 | 2244917_24.patch | 11.31 KB | Mile23 |
#19 | interdiff.txt | 5.87 KB | Mile23 |
#19 | 2244917_19.patch | 11.48 KB | Mile23 |
| |||
#16 | interdiff.txt | 1.62 KB | Mile23 |
#16 | 2244917_16.patch | 10.69 KB | Mile23 |
|
Comments
Comment #1
xjmComment #2
sunComment #3
alexpottWe're removing test coverage - but we don't seem to be adding any?
Comment #5
sunLOL
(53 chars)
Comment #6
xjmIf this is a hard blocker for that, then it should also be a beta blocker. I'm not clear on why this needs to be a hard blocker, though.
Comment #7
jhedstromNot sure if this is still a major task or not, but I did a quick reroll, and added a test. I'd imagine this change will break other tests...
As I was writing the test, I wondered if skipped extensions should be logged, or otherwise called out? Silently skipping might be a DX regression.
Comment #9
XanoRe-roll.
Comment #11
XanoComment #12
XanoThe last patch removes
ExtensionNameLengthException
, because it wasn't used anywhere anymore. However, I believe we should keep it and throw it inExtensionDiscovery
rather than introducing silent failures that don't help anybody.Comment #14
Mile23I wasn't able to easily reroll, and I disagree with the implementation, so I just re-did it.
Agreed that it should throw an exception.
Turned it into a class method, so that we can test it without a lot of dependencies.
Comment #16
Mile23Woops... Missed the test from the patch.
Comment #18
xjmWe can no longer remove the global constant in Drupal 8, but we can deprecate it in favor of the class constant. Moving to 8.1.x rather than 9.x to implement that solution. Thanks @Mile23.
Comment #19
Mile23Thanks, @xjm.
OK, so the behavior here is changed such that
Drupal\Tests\system\Kernel\Extension\ModuleHandlerTest
has a lot of failures.That's because every time one of the tests runs, SimpleTest dutifully tries to discover all the test modules, which results in an exception because it tries to discover a module named
invalid_module_name_over_the_maximum_allowed_character_length
.This means that we have a @todo here, to refactor that test to place the illegally-named module in a place where it will only be discovered for the test of its behavior.
Comment #24
Mile23This patch turns the test into a
KernelTestBase
test, because it also introduces a coupling betweenExtensionDiscovery
andcore/includes/bootstrap.inc
.This patch will fail miserably because it works. :-) How? Journey with me now, down the rabbit hole...
We have a test module in
core/modules/system/tests/modules/
calledinvalid_module_name_over_the_maximum_allowed_character_length
. It's used byDrupal\Tests\system\Functional\Module\InstallTest
. It correctly tests whether this long module name will throw an exception, allowing for falsifiability by having the module present in the file system.However, this is discovered by
TestDiscovery
on every test discovery run, which causes an exception.Now, that'd be fine, because we could remove that part of
InstallTest
and inject a virtual file system intoModuleInstaller
with a long module name, right?No, we can not. Because right smack in the middle of
ModuleInstaller::install()
, we see:So unless we replicate install.inc in our virtual file system, we can't determine whether
ModuleInstaller
throws the proper exception or not. This seems like a violation of the thing we're trying to test.Thus, here we are. This issue should really either be postponed on or marked duplicate of #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList