Problem

  1. Various code spots throughout core are validating the length of extension names, whereas...

  2. An extension with a too long name MUST NOT enter the application in the first place.

Proposed solution

  1. Centralize extension name length validation into ExtensionDiscovery.

CommentFileSizeAuthor
#24 2244917_24.patch11.31 KBMile23
#19 interdiff.txt5.87 KBMile23
#19 2244917_19.patch11.48 KBMile23
#16 interdiff.txt1.62 KBMile23
#16 2244917_16.patch10.69 KBMile23
#14 2244917_14.patch9.06 KBMile23
#11 drupal_2244917_11.patch18.65 KBXano
#11 interdiff.txt11.71 KBXano
#9 drupal_2244917_9.patch8.77 KBXano
#7 extension-name-length-2244917-07.patch10.51 KBjhedstrom
#7 interdiff.txt1.63 KBjhedstrom
drupal8.ext-length.0.patch8.35 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Naming things is hard
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Assigned: Unassigned » sun
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Needs work
Issue tags: +Needs tests

We're removing test coverage - but we don't seem to be adding any?

The last submitted patch, drupal8.ext-length.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

LOL

system_incompatible_module_version_dependencies_test

(53 chars)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

If 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.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Assigned: sun » Unassigned
Status: Needs work » Needs review
FileSize
interdiff.txt1.63 KB
extension-name-length-2244917-07.patch10.51 KB

Not 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.

Status: Needs review » Needs work

The last submitted patch, 7: extension-name-length-2244917-07.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
drupal_2244917_9.patch8.77 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 9: drupal_2244917_9.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
interdiff.txt11.71 KB
drupal_2244917_11.patch18.65 KB
Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

The last patch removes ExtensionNameLengthException, because it wasn't used anywhere anymore. However, I believe we should keep it and throw it in ExtensionDiscovery rather than introducing silent failures that don't help anybody.

Status: Needs review » Needs work

The last submitted patch, 11: drupal_2244917_11.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2244917_14.patch9.06 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, 14: 2244917_14.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2244917_16.patch10.69 KB
interdiff.txt1.62 KB

Woops... Missed the test from the patch.

Status: Needs review » Needs work

The last submitted patch, 16: 2244917_16.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.0.x-dev » 8.1.x-dev

We 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.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
2244917_19.patch11.48 KB
interdiff.txt5.87 KB

Thanks, @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.

Status: Needs review » Needs work

The last submitted patch, 19: 2244917_19.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2244917_24.patch11.31 KB

This patch turns the test into a KernelTestBase test, because it also introduces a coupling between ExtensionDiscovery and core/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/ called invalid_module_name_over_the_maximum_allowed_character_length. It's used by Drupal\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 into ModuleInstaller with a long module name, right?

No, we can not. Because right smack in the middle of ModuleInstaller::install(), we see:

    // Required for module installation checks.
    include_once $this->root . '/core/includes/install.inc';

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

Status: Needs review » Needs work

The last submitted patch, 24: 2244917_24.patch, failed testing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.