Problem/Motivation

Core does not restrict the maximum size of extension-names (names of themes and modules) in any way; though it is actually (with inconsistent numbers) restricted in various database tables of core modules.

Proposed resolution

Restrict the number of characters extension-names can have.

Remaining tasks

[✓] Write a patch that restricts the module/theme size by throwing an exception; also add a testcase for this
[✓] Change the table columns containing the module/theme names and add upgrade paths for the existing tables
[✓] Add an upgrade path test
[✓] Document possible issues during the upgrade of an existing site and how to resolve them (Change record) See #115
[✓] Add a link to the documentation page when checking the upgrade requirements to this patch See #116
[✓] Add a validation handler for throwing a form error when a theme or module with a too long name is tried to be enabled
[✓] core gates docs minimal requirements review
[✓] Manual testing. See #124-129
[✓] Add test that will fail for enabling module with too long name in the UI. See #139.
[✓] Review the most current patch; If everything is fine: RTBC!

User interface changes

None.

API changes

Extensions (themes and modules) won't be able to use shortnames longer than 50 characters.
There are no such long names in (contrib) modules/themes known on drupal.org, though this might be an issue for custom modules, such as features.

#1990544: Convert system_modules() to a Controller
#1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route

Follow-ups

#2020939: Add a requirements check for the 50 character restricted module and theme name lengths

Original report by chx

I think 64 is enough. Anyone ever saw a longer one?
drupal.org record is http://drupal.org/project/db_export_edit_mysql_row_as_text_and_import at 43 characters.

CommentFileSizeAuthor
#148 module-name-1852454-148.patch18.21 KBYesCT
#148 interdiff-143-148.txt1.39 KBYesCT
#143 module-name-1852454-143.patch18.2 KBYesCT
#143 interdiff-139-143.txt4.07 KBYesCT
#139 module-name-1852454-139-shouldfail-addsUItest-noUIfix.patch17.81 KBYesCT
#139 interdiff-120-139-fail.txt908 bytesYesCT
#135 module-name-1852454-135.patch17.64 KBYesCT
#135 interdiff-120-134.txt2.31 KBYesCT
#126 link-to-toolong.png223.3 KBYesCT
#120 module-name-1852454-120.patch17.39 KBYesCT
#120 interdiff-116-120.txt1 KBYesCT
#116 module-name-1852454-116.patch17.51 KBpatrickd
#116 interdiff.txt1.18 KBpatrickd
#113 module-name-1852454-113.patch19.55 KBjessebeach
#112 module-name-1852454-112.patch17.33 KBpatrickd
#99 module-name-1852454-99.patch17.28 KBpatrickd
#99 interdiff.txt1.2 KBpatrickd
#98 diff-90-97.txt7.51 KBpatrickd
#97 module-name-1852454-97.patch17.17 KBdjroshi
#96 module-name-1852454-96.patch17.96 KBdjroshi
#95 module-name-1852454-95.patch17.46 KBWim Leers
#94 module-name-1852454-94.patch17.47 KBpatrickd
#91 module-name-1852454-90.patch17.53 KBBerdir
#91 module-name-1852454-90-interdiff.txt5.89 KBBerdir
#90 w3Nbr.gif1.67 MBchx
#84 module-name-1852454-84.patch11.63 KBpatrickd
#84 module-name-1852454-84-interdiff.txt1.43 KBpatrickd
#83 module-name-1852454-83.patch11.64 KBBerdir
#83 module-name-1852454-83-interdiff.txt3.11 KBBerdir
#81 module-name-1852454-81.patch9.59 KBPancho
#72 module-name-1852454-72.patch11.64 KBPancho
#70 module-name-1852454-69.patch11.06 KBxjm
#69 module-name-interdiff.txt5.11 KBxjm
#67 module-name-1852454-67.patch12.51 KBxjm
#67 lol.txt223 bytesxjm
#65 module-name-1852454-65.patch12.55 KBxjm
#65 interdiff.txt486 bytesxjm
#62 module-name-1852454-62.patch12.5 KBxjm
#62 interdiff-module-name.txt10.09 KBxjm
#52 module-name-1852454-52.patch11.87 KBpatrickd
#48 module-name-1852454-48.patch12.49 KBdealancer
#46 module-name-1852454-46.patch10.43 KBBerdir
#46 module-name-1852454-46-interdiff.txt1.5 KBBerdir
#44 module-name-1852454-44.patch9.8 KBamateescu
#44 interdiff.txt1.31 KBamateescu
#42 module-name-1852454-42.patch9.63 KBamateescu
#42 interdiff.txt7.03 KBamateescu
#40 module-name-1852454-40-pass.patch9.84 KBpatrickd
#38 interdiff.txt2.17 KBpatrickd
#38 module-name-1852454-38-pass.patch6.69 KBpatrickd
#32 module-name-1852454-32-fail.patch2.44 KBxjm
#32 module-name-1852454-32-pass.patch4.52 KBxjm
#30 module-name-1852454-30-tests.patch999 bytesxjm
#30 module-name-1852454-30.patch3.06 KBxjm
#23 module-name-1852454-23.patch2.08 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

andypost’s picture

Great! don't forget about db-schema some DBs has limit on table names and views hits this limit for aliases #571548: Identifiers longer than 63 characters are truncated, causing Views to break on Postgres

idflood’s picture

64 is sane, but what would be the overhead to make it to 128 or even 256? If the difference is small then I think it would be better.

128/256 over 64 would have these benefitse:

  • Much more room for super_crazy_long_module_name
  • Avoid having an issue some times later to remove the limit/increase it
  • A lot of common filesystems have a limit of 255 char or 255 bytes, having the same limit for module name seems coherant to me 1

1 http://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits

tstoeckler’s picture

Re #3: The problem is that there is a maximum length on DB indexes (333 IIRC). So if you have a compound index which consists of the module name *and* something else, then you can easily hit that limit if the module name is already 255. See (the latest comments of) #347988: Move $user->data into own table for more information (as linked in #1).

Yaron Tal’s picture

The overhead would be that some people might actually use all 255 characters and that some day you'll have to implement hook_module_for_importing_views_from_features_and_export_them_in_json_xml_csv_or_pdf_format_for use_in_presentations_or_when_releasing_stuff_to_a_test_or_production_server_with_the_features_import_module
_installed_or_with_the_views_import_functionality_view_export_alter().

If there is a limit, better make it a sane limit (although I think 64 might already by too much).

idflood’s picture

#4: Sorry, didn't saw comment #2 when i posted #3. I was just pointing that it would be better to target the largest limit we can. 63 is perfectly fine for me : )

I think the limit should correspond to the first maximum length it will hit. So the db seems to gives a limitation of 63, even less if there is modulename_+something . Anything else?

andypost’s picture

Also we already fight a lot with primary key length limits for example #1280792: Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

zhangtaihao’s picture

Title: Define a max module name length » Does anyone have a use case for a longer than 64 character module name?

I propose a deadline be set for resolving the question, esp. given it's now on Planet Drupal. Otherwise, this becomes a Flying Spaghetti Monster with no proof in sight.

chx’s picture

Issue summary: View changes

Updated issue summary.

zhangtaihao’s picture

Also, for Drupal 8+ should we consider the availability of PHP namespaces? In the past, the length of a module's name generally determined the readability of class names (e.g. SuperDuperLongModuleNameSpecialFunctionalityClass). With PHP 5.3 (since Drupal 8), a module name is now part of the namespace of a class.

xjm’s picture

Title: Does anyone have a use case for a longer than 64 character module name? » Define a max module name length

Thanks everyone! For now, let's focus on identifying whether there are any module names longer than the longest d.o contrib, which is 43 characters.

xjm’s picture

Title: Define a max module name length » Does anyone have a use case for a longer than 64 character module name?
sun’s picture

The longest module name on d.o in #0 is already insanely long enough, IMHO.

If anyone has a custom module that exceeds the limit, then there's a trivial solution: Rename the module.

So it's not like the world would end.

In light of that, even 64 feels too much to me. We could further decrease to 50. But also fine with 64.

Anonymous’s picture

I found system_incompatible_module_version_dependencies_test in my copy of contrib. But that's only 53 characters, so should be fine.

dalin’s picture

In this thread everyone seems to be talking about machine names, but in the Drupal Planet post that chx posted he seemed to be talking about human names
http://groups.drupal.org/node/270248

Which one are we talking about?

I have a module with a long human name (68 chars), but a short machine name:
http://drupal.org/project/cd_sunlight

tstoeckler’s picture

Re #17: This is only about the internal/machine-name, not the human-visible name.

jonhattan’s picture

Status: Active » Needs review

Lorger in the update feed is 43 chars:

wget http://updates.drupal.org/release-history/project-list/all
grep "<short_name>" all | sed 's@<\([^<>][^<>]*\)>\([^<>]*\)</\1>@\2@g' | tr -s " "  | wc -L

Result is 44 chars but lines have a one whitespace prefix for some reason.

xjm’s picture

Title: Does anyone have a use case for a longer than 64 character module name? » Restrict module and theme name lengths to 64 characters
Assigned: Unassigned » xjm
Status: Needs review » Active

Alright, let's make a decision here. I'll roll a patch.

xjm’s picture

In light of several situations where we might end up with multiple module or theme names in configuration object names, 64 seems too big. The following projects have shortnames over 32:

33	commerce_add_to_cart_confirmation
33	commerce_price_decimals_formatter
33	hierarchical_select_menu_position
33	private_files_download_permission
33	views_filterblock_fieldset_remove
34	commerce_migrate_ubercart_shipping
34	node_reference_set_trail_formatter
35	commerce_payment_balance_validation
35	services_resource_extension_content
35	twitter_bootstrap_panels_everywhere
35	views_taxonomy_hierarchy_top_filter
36	entityreference_dynamicselect_widget
36	og_multiple_mandatory_groups_by_role
36	services_taxonomy_resource_extension
37	dsero_anti_adblock_for_google_adsense
38	conditional_roles_workbench_moderation
42	subscriptions_authorscontentreferencedterm
43	db_export_edit_mysql_row_as_text_and_import

That doesn't include submodules that might have longer names (like the test module @linclark found) but those are less problematic to rename than project shortnames.

xjm’s picture

Title: Restrict module and theme name lengths to 64 characters » Restrict module and theme name lengths to 40 characters

The longest that has a release is 37 characters, so let's call it 40.

xjm’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2.08 KB

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-23.patch, failed testing.

andypost’s picture

Hey, 40 seems resonable but what about "features"? Thay could have a longer names so #2 states about 63 as limit

andypost’s picture

Status: Needs work » Needs review
xjm’s picture

The problem with 64 characters is that if we have a pattern for configuration entity names of (e.g.) owner_module.api_module.entity_type.entity_ID, or something similar to block name patterns of owner_module.api_module.block.display_ID.block_ID, that doesn't fit in 250 characters which is the max for config object name lengths due to filesystem limitations. If we scale back modules, both patterns fit. Also, features won't have a straight upgrade path anyway.

dealancer’s picture

Status: Needs review » Needs work

Hey guys!

Drupal search index restricts maximun length of the module name to 16 chars which is too small. See #1425622: Increase length of 'type' field in search_dataset, search_index tables from 16 to 64. The current patch looks good however it does not solve some problems. So we need to re roll patch #1425622 as it is very related.

There is also related issue in Features module, it is about 100 chars restriction to the file name, but I already expected this issue with a 50 chars file name. See #1529066: Features implementation of tar truncates file-paths at 100 characters for more info. Though it looks like it is fixed in I guess this is fixed in the new 7.x-1.x version of the Features.

Updated status to needs work as we have new circumstances. Need to discuss this.

xjm’s picture

Updated status to needs work as we have new circumstances. Need to discuss this.

The change is in 8.x only, so we don't need to worry about what 7.x modules do other than making sure the upgrade path isn't impossible.

However, this is still NW for tests, which I'll post shortly.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
999 bytes

So, system_incompatible_module_version_dependencies_test is not in contrib, it's in core; as is system_incompatible_core_version_dependencies_test. #23 presumably passes because these modules are only used in DependencyTest to verify that dependency checking is marked correctly on the form; they are not actually enabled.

This could still use a test for a theme name. We might also want to update the database schema to a 40-character varchar and shorten those extra-long test module names.

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-30.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
2.44 KB

Oops, #30 is incomplete.

andypost’s picture

I think better prevent users to try to enable modules with disallowed length from module list!

+++ b/core/includes/module.inc
@@ -465,6 +465,13 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+        throw new \Exception(format_string('Module name %name is over the maximum allowed length of @max characters.', array(

+++ b/core/includes/theme.inc
@@ -1515,6 +1515,14 @@ function theme_enable($theme_list) {
+      throw new \Exception(format_string('Theme name %name is over the maximum allowed length of @max characters.', array(

Suppose we need a visual indication in module/themes list too. Like we have for modules with incompatible core version

xjm’s picture

Or maybe even from system_register(). I'm not confident that module_enable() is the right place.

Suppose we need a visual indication in module/themes list too. Like we have for modules with incompatible core version.

Ah, good suggestion. Though, if we throw the exception when registering the module, the user will never see that page.

lpalgarvio’s picture

63/64 would be optimal in order to allow expansability/flexibility, but at very least, i ask >=48, please.

so i can maintain stuff like this:

PROJECT_features_ctools_layouts
	PROJECT_features_ctools_layouts_footer,
	PROJECT_features_ctools_layouts_header

PROJECT_features_ctools_mini
	PROJECT_features_ctools_mini_footer,
	PROJECT_features_ctools_mini_header

PROJECT_features_ctools_pages
	PROJECT_features_ctools_pages_apoios,
	PROJECT_features_ctools_pages_artistas,
	PROJECT_features_ctools_pages_atividades,
	PROJECT_features_ctools_pages_comunicados,
	PROJECT_features_ctools_pages_eventos,
	PROJECT_features_ctools_pages_exposicoes,
	PROJECT_features_ctools_pages_faqs,
	PROJECT_features_ctools_pages_frontpage,
	PROJECT_features_ctools_pages_galerias,
	PROJECT_features_ctools_pages_noticias,
	PROJECT_features_ctools_pages_obras,
	PROJECT_features_ctools_pages_parcerias,
	PROJECT_features_ctools_pages_publicacoes,
	PROJECT_features_ctools_pages_recortes,
	PROJECT_features_ctools_pages_recursos

PROJECT_features_ctools_panes
	PROJECT_features_ctools_panes_apoios,
	PROJECT_features_ctools_panes_artistas,
	PROJECT_features_ctools_panes_atividades,
	PROJECT_features_ctools_panes_comunicados,
	PROJECT_features_ctools_panes_eventos,
	PROJECT_features_ctools_panes_exposicoes,
	PROJECT_features_ctools_panes_faqs,
	PROJECT_features_ctools_panes_frontpage,
	PROJECT_features_ctools_panes_galerias,
	PROJECT_features_ctools_panes_general,
	PROJECT_features_ctools_panes_noticias,
	PROJECT_features_ctools_panes_obras,
	PROJECT_features_ctools_panes_parcerias,
	PROJECT_features_ctools_panes_publicacoes,
	PROJECT_features_ctools_panes_recortes,
	PROJECT_features_ctools_panes_recursos

PROJECT_features_data_tables
	PROJECT_features_data_tables_artists,
	PROJECT_features_data_tables_works

PROJECT_features_flag
	PROJECT_features_flag_node_frontpage

PROJECT_features_node
	PROJECT_features_node_apoio,
	PROJECT_features_node_atividade,
	PROJECT_features_node_basicpage,
	PROJECT_features_node_comunicado,
	PROJECT_features_node_evento,
	PROJECT_features_node_exposicao,
	PROJECT_features_node_faq,
	PROJECT_features_node_galeria,
	PROJECT_features_node_noticia,
	PROJECT_features_node_parceria,
	PROJECT_features_node_publicacao,
	PROJECT_features_node_recorte,
	PROJECT_features_node_recurso,
	PROJECT_features_node_webform

PROJECT_features_rules
	PROJECT_features_rules_apoio,
	PROJECT_features_rules_atividade,
	PROJECT_features_rules_comunicado,
	PROJECT_features_rules_evento,
	PROJECT_features_rules_exposicao,
	PROJECT_features_rules_faq,
	PROJECT_features_rules_galeria,
	PROJECT_features_rules_noticia,
	PROJECT_features_rules_parceria,
	PROJECT_features_rules_publicacao,
	PROJECT_features_rules_recorte,
	PROJECT_features_rules_recurso

PROJECT_features_styles
	PROJECT_features_styles_field_image_banner,
	PROJECT_features_styles_field_image_gallery,
	PROJECT_features_styles_field_image_thumbnail,
	PROJECT_features_styles_views_colecao
	PROJECT_features_styles_views_highlights,
	PROJECT_features_styles_views_publicacoes

PROJECT_features_taxonomy
	PROJECT_features_taxonomy_node_apoio,
	PROJECT_features_taxonomy_node_atividade,
	PROJECT_features_taxonomy_node_comunicado,
	PROJECT_features_taxonomy_node_evento,
	PROJECT_features_taxonomy_node_exposicao,
	PROJECT_features_taxonomy_node_faq,
	PROJECT_features_taxonomy_node_galeria,
	PROJECT_features_taxonomy_node_noticia,
	PROJECT_features_taxonomy_node_parceria,
	PROJECT_features_taxonomy_node_publicacao,
	PROJECT_features_taxonomy_node_recorte,
	PROJECT_features_taxonomy_node_recurso,
	PROJECT_features_taxonomy_other_personalidade,
	PROJECT_features_taxonomy_other_seccao

PROJECT_features_user
	PROJECT_features_user_build_seccoes

PROJECT_features_views
	PROJECT_features_views_apoios,
	PROJECT_features_views_artistas,
	PROJECT_features_views_atividades,
	PROJECT_features_views_comunicados,
	PROJECT_features_views_eventos,
	PROJECT_features_views_exposicoes,
	PROJECT_features_views_faqs,
	PROJECT_features_views_galerias,
	PROJECT_features_views_highlights,
	PROJECT_features_views_image_gallery,
	PROJECT_features_views_noticias,
	PROJECT_features_views_obras,
	PROJECT_features_views_parcerias,
	PROJECT_features_views_publicacoes,
	PROJECT_features_views_recortes,
	PROJECT_features_views_recursos

this spans in a lot of my projects and i've been maintaining this consistency for some years

lpalgarvio’s picture

also, please consider that Features module truncation has been fixed.
#1529066: Features implementation of tar truncates file-paths at 100 characters

adammalone’s picture

Why not incorporate the check for name length into system_modules. That's where core incompatibility is checked. It would also make sense to put it there as the end user will be given a very obvious message and simply will not be allowed to enable the module, which is friendlier than having an error thrown.

patrickd’s picture

Sets the core's module-columns to the maximum module length

amateescu’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/file.install
@@ -112,7 +112,7 @@ function file_schema() {
+        'length' => 40,

Is there a reason to not use the DRUPAL_MODULE_NAME_MAX_LENGTH constant here and all the other changes from your interdiff?

And now that I see it, that constant should apply to themes and installation profiles as well, so its name seems a bit wrong.. maybe DRUPAL_EXTENSION_NAME_MAX_LENGTH?

patrickd’s picture

Status: Needs work » Needs review
FileSize
9.84 KB

Using constants in schemas didn't feel right to me ;) But I guess it wont hurt

- Added DRUPAL_MODULE_NAME_MAX_LENGTH constant to schemas
- Added upgrade path for updating the columns
- Also converted length of theme name (only in users table)

Tested installation with patch applied and installation without patch + update.php locally.

amateescu’s picture

Status: Needs review » Needs work

So.. I think that using the constant in hook_schema() is fine because that always needs to be up to date with the latest value, but in update functions we should be specific about what we update to, so no constant usage in there :) Also, why do we need three separate functions in user.install?

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.03 KB
9.63 KB

Implemented my suggestions from #41 and renamed the constant to DRUPAL_EXTENSION_NAME_MAX_LENGTH. We'll have the same upgrade path failures due to the update function from menu_link.module that won't run because of #1239370: Module updates of new modules in core are not executed.

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-42.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
9.8 KB

Waaait a minute :) We already have system_update_8048() which messes with the {menu_links} table (from the menu links to entities conversion patch), and since we don't support HEAD to HEAD upgrades atm we can just update the module column in there.

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-44.patch, failed testing.

Berdir’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
1.5 KB
10.43 KB

The test module info file was still using the old format. This should fix the test.

It seems this is blocking #1969680: Installation fails with MyISAM - key too long, so raising priority as this is in that case blocking a critical task and a critical bug.

dealancer’s picture

Status: Needs review » Needs work

Looks good, but there is one more place where we need to update the database field widths from 16 to DRUPAL_EXTENSION_NAME_MAX_LENGTH). It is search module! See #1425622: Increase length of 'type' field in search_dataset, search_index tables from 16 to 64. Let's re roll this patch

dealancer’s picture

Status: Needs work » Needs review
FileSize
12.49 KB

Here we go! Updated hook_install and added hook_update_N for the search module.

patrickd’s picture

Assigned: xjm » Unassigned
Status: Needs review » Needs work

The last patch defines an SchemaIndexLengthException - I don't think this belongs in here (it's even never used)

That exception should rather be introduced with #1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL instead, this issue is only about the module length - and as a side-effect it fixes the key-too-long issue mentioned

dealancer’s picture

Need the feedback from @Berdir, cause it was introduced even earlier in #46.

Berdir’s picture

Yeah, sure, remove that thing :)

patrickd’s picture

Status: Needs work » Needs review
FileSize
11.87 KB

Removed the SchemaIndexLengthException class file

The last submitted patch, module-name-1852454-52.patch, failed testing.

dealancer’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

#52: module-name-1852454-52.patch queued for re-testing.

andypost’s picture

Issue summary: View changes

Copy of the revision from November 28, 2012 - 05:04.

andypost’s picture

Status: Needs review » Needs work

I think better to not allow enable modules on list page as we have for core-incompatible version of the module
see system_modules()

EDIT added related #1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route & #1990544: Convert system_modules() to a Controller

+++ b/core/includes/module.incundefined
@@ -260,6 +260,13 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+      if (strlen($module) > DRUPAL_EXTENSION_NAME_MAX_LENGTH) {
+        throw new \Exception(format_string('Module name %name is over the maximum allowed length of @max characters.', array(
+          '%name' => $module,
+          '@max' => DRUPAL_EXTENSION_NAME_MAX_LENGTH,
+        )));

suppose this not fatal?

xjm’s picture

How is it not fatal?

xjm’s picture

Title: Restrict module and theme name lengths to 40 characters » Restrict module and theme name lengths to 50 characters

From the comments above, it sounds like we need to increase it to 50. That's still sufficiently small for configuration object name lengths to fit two of them with two 64-character entity machine names and some change

xjm’s picture

Assigned: Unassigned » xjm
+++ b/core/includes/bootstrap.incundefined
@@ -188,6 +188,11 @@
+ * The maximum number of characters in a module or theme name.
+ */
+const DRUPAL_EXTENSION_NAME_MAX_LENGTH = 40;

Let's increase this to 50, and also add a paragraph to the docblock explaining why it matters (configuration object names, schema indexes, etc.)

+++ b/core/includes/module.incundefined
@@ -260,6 +260,13 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+        throw new \Exception(format_string('Module name %name is over the maximum allowed length of @max characters.', array(

+++ b/core/includes/theme.incundefined
@@ -1560,6 +1560,14 @@ function theme_enable($theme_list) {
+      throw new \Exception(format_string('Theme name %name is over the maximum allowed length of @max characters.', array(

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleEnable.phpundefined
@@ -44,4 +44,19 @@ function testRequiredModuleSchemaVersions() {
+    catch (\Exception $e) {

We should use a typed exception here. Not the Schema exception; our own typed exception. :)

+++ b/core/modules/file/file.installundefined
@@ -288,3 +288,17 @@ function file_update_8002() {
+    'length' => '40',

Let's use the constant in spots like these.

+++ b/core/modules/node/node.installundefined
@@ -752,6 +752,20 @@ function node_update_8015() {
+function node_update_8016() {
+  $spec = array(
+    'description' => 'The module defining this node type.',
+    'type' => 'varchar',
+    'length' => '40',
+    'not null' => TRUE,
+  );
+  db_change_field('node_type', 'module', 'module', $spec);

Do we want to wrap all these updates in db_field_exists() checks for a more defensive upgrade path?

+++ b/core/modules/node/node.installundefined
@@ -752,6 +752,20 @@ function node_update_8015() {
+}
+
+
+/**
  * @} End of "addtogroup updates-7.x-to-8.x"
  * The next series of updates should start at 9000.

Extra blank line here.

+++ b/core/modules/search/search.installundefined
@@ -214,3 +214,23 @@ function search_update_8001() {
+ * Use max allowed module name size for type fields in {search_dataset},
+ * {search_index} and {search_node_links}.

+++ b/core/modules/user/user.installundefined
@@ -1051,5 +1051,38 @@ function user_update_8017() {
+ * Convert the 'module' column in {role_permission} and {users_data} and the
+ * 'theme' column in {users_data} to the maximum shortname length.

These should be one line of 80 characters or fewer.

I think @andypost is also asking that the module page display a warning that the module is incompatible before you enable it.

I'll work on these things.

xjm’s picture

We also probably need to add a check during the ugprade path somehow to ensure that any module N that's enabled doesn't have a length more than the max, and we should add upgrade path tests.

xjm’s picture

Using constants in schemas didn't feel right to me ;) But I guess it wont hurt

Ah, actually, you have a point here--not for the schemas, but for the update hooks rather. The value could change in the future, which would change the schema and update hook values. We probably need to define a constant (in system.install I guess?) for the schema version max length.

xjm’s picture

The change in #47 and #48 is out of scope here, and also incorrect. Those fields are not module names; they are entity type and bundle names. See #1709960: declare a maximum length for entity and bundle machine names.

xjm’s picture

Just the cleanups from #58 to #61 for now. I'll look into using the requirements for install and update next.

xjm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-62.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
486 bytes
12.55 KB

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-65.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
223 bytes
12.51 KB

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-67.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
5.11 KB

Yeah, okay, any constant in the upgrade path is a bad idea, especially referencing one in system.install from other install files. ;) The fact that more things didn't fail there indicates that we definitely need upgrade path tests...

xjm’s picture

FileSize
11.06 KB

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-69.patch, failed testing.

Pancho’s picture

Status: Needs work » Needs review
FileSize
11.64 KB

Weird, but a name length of 48 sounds kind of more "natural" in our artificial binary world.
Anyway nice that you're shortening keys, because we need to be more thrifty when it comes to key lengths, see #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols).

Rerolling with ExtensionNameLengthException.php missing in #70.
Hoped that was the reason for the failing test, but maybe something else is missing, because my local test still failed.

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-72.patch, failed testing.

xjm’s picture

Because of #1976110: D8 upgrade path: file module; Update #8001, we'll need to either move the file_usage size adjustment back into the earlier update hook, or... something...

xjm’s picture

@Pancho, can you provide an interdiff showing what changes you made?

48 isn't special in binary. It's a multiple of 3.

xjm’s picture

@@ -0,0 +1,17 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Extension\ExtensionNameLengthException.
+ */
+
+namespace Drupal\Core\Extension;
+
+use Exception;
+
+/**
+ * Exception thrown when the extension's name length exceeds the allowed maximum.
+ */
+class ExtensionNameLengthException extends Exception {
+
+}

Oh, I see now. I guess I didn't stage this file somehow. #fail

Note that we do not use global namespace classes like Exception. Instead, you should reference it as \Exception. Reference: http://drupal.org/node/1353118

Edit: Also, note that I have the issue assigned to myself, which means I'm working on it actively locally. :)

Pancho’s picture

FAILED: [[SimpleTest]]: [MySQL] 55,592 pass(es), 2 fail(s), and 0 exception(s).

Note that the second test failing in #72 actually is a bug in a simpletest. So we're still with the 1 fail in #70.
Makes me wonder why the missing class definition of the thrown Exception in #70 doesn't show up in the tests.

48 isn't special in binary.

I know, it's just 2⁵+2⁴ = 32+16 = 48. Don't bother, it looks more beautiful to me, but is completely irrelevant.

Note that we do not use global namespace classes like Exception.

grep -r 'use Exception' * gives 35 classes that are use'ing Exception. I'm gonna file another issue for that.
[edit:] here it is: #1993370: Fully qualify \Exception instead of "use"ing it

Pancho’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path tests

#72: module-name-1852454-72.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs upgrade path tests

The last submitted patch, module-name-1852454-72.patch, failed testing.

xjm’s picture

@Pancho, I don't think it's a bug in simpletest. Could you elaborate what you're basing that on?

Thanks!

Pancho’s picture

Status: Needs work » Needs review
FileSize
9.59 KB

Using \Exception instead of 'use Exception'.
Sorry, no interdiff again, but this is the only change I made.

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-81.patch, failed testing.

Berdir’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
3.11 KB
11.64 KB

Re-roll, the last patch was missing the exception and new module files again :)

Also added the type: module to the info file, that's why it didn't find the module and didn't throw an exception. The class name in the test also wasn't consistent. And updated the system update function number.

This should pass.

patrickd’s picture

Small coding standard changes..

What exactly do we have to test on this upgrade path? Test whether the fields' size change properly?

Berdir’s picture

Yes, exactly. Either add a new upgrade test or extend an existing one and verify that the schema changes were made. As there is no standard way to get that information out of there, you should probably simply try to insert something that's too long and make sure it throws an exception.

patrickd’s picture

Assigned: Unassigned » patrickd
patrickd’s picture

If we upgrade an existing site with existing - too long - module/theme names in the database, no exception will be thrown as this can only happen on module/theme_enable() - but they are already enabled.

If this is about whether fields were properly altered - isn't this about testing db_change_field()?
The fields wont be properly altered if that function doesn't work, that's a complete other issue in my eyes?

Berdir’s picture

We don't want to test if db_change_field() works correctly, we want to test that it was called and that it was called with the right arguments.

Existing values is a good point, not sure what to do about that.

patrickd’s picture

Assigned: patrickd » Unassigned

This sounds like a case for chx?^^

chx’s picture

FileSize
1.67 MB

If there's a module name in system greater than 50 chars, just in requirements.

Do not write a test for the case when the module name is too long in file_usage and stuff if it's not trivial (which it is not). There's no point in wasting time.

Berdir’s picture

Ok, so added that requirements check and tests for both the upgrade path and that requirement check.

patrickd’s picture

Thank you very much berdir!

About the ModuleNameLengthUpgradePathTest testcase:

Though the file_usage table is the table making problems, it's not the only table that is altered by this patch.

Also chx says *Do not write a test for the case when the module name is too long in file_usage*..

So I guess we should rather either write this test for every changed table, or for none of them?

Berdir’s picture

It's not the only table that's altered but a few of the other ones will go away, so it won't matter anymore (e.g. node_type). But it should be easy to duplicate the pattern for the other tables, I was just too lazy to do that :)

You missed the second part of chx's sentence, which is "if it's not trivial". And that's referencing to the second test, about existing modules in the database, for which I didn't add test coverage to file_usage but system :)

patrickd’s picture

Patch did not apply anylonger, rerolled against HEAD

Berdir has done an awesome job so far, could anyone (more experienced than me) could have a look at this patch please? :)

Wim Leers’s picture

FileSize
17.46 KB

Straight reroll of #94 to no longer trigger Fatal error: Cannot redeclare node_update_8016() (previously declared in /home/sb98000010592ff4/www/core/modules/node/node.install:1086) in /home/sb98000010592ff4/www/core/modules/node/node.install on line 1162

Simply renamed the update function added in #94 from 8016 to 8019.

djroshi’s picture

FileSize
17.96 KB

#95 fails to apply. Attempting a reroll of #94

djroshi’s picture

FileSize
17.17 KB

Sorry #96 wasn't much better. Hoping this one works...

patrickd’s picture

FileSize
7.51 KB

Here's a diff between the last patch of Berdir (#90) and all the rerolls (#97).

Moved the module name size length check from module_enable() to ModuleHandler::enable()
#90 -> #95: changed node_update_8016() to node_update_8019()
#95 -> #97: moved logic from node_update_8019() into node_update_8016() why?

patrickd’s picture

This patch moves the change on the node_type table back into its own update function.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Code looks great and the patch provides very thorough upgrade tests. Let's do it.

djroshi’s picture

Actually I didn't move anything. I rerolled #94 => #97 (#95 didn't apply) and left the logic code in node_update_8016, primarily to avoid future merge conflicts if/when this needs to be rerolled. Agree it should live in it's own update function - sorry I was in a hurry to test something on simplytest.me :)

patrickd’s picture

@tstoeckler, great, thanks for your review!

@djroshi, I see but rather than avoiding merge conflicts we should build it correctly right now so we can get it in earlier (also always try to build on the latest patch, even if it does not apply, so nothing gets lost in the process), but I get your point, thanks for your work!

catch’s picture

Chances are probably quite slim but do we need to account for the case where two modules are both > 50 chars long and they end up with the same name once truncated? Otherwise this looks good to me.

patrickd’s picture

As far I can see there is no truncation of too long module names.

function update_prepare_d8_bootstrap() {
       // Migrate each extension into configuration, varying by the extension's
       // status, and record its schema version.
       foreach ($result as $record) {
+        if (drupal_strlen($record->name) > 50) {
+          $requirements['module name too long ' . $record->name] = array(
+            'title' => 'Module name too long',
+            'value' => format_string('@name is @count characters long.', array('@name' => $record->name, '@count' => drupal_strlen($record->name))),
+            'description' => 'Module names longer than 50 characters are no longer supported.',
+            'severity' => REQUIREMENT_ERROR,
+          );
+          update_extra_requirements($requirements);
+        }
+

Instead it will complain about a requirement error if an extension with a too long name is detected and wont let people upgrade.

So there's no such issue ?

catch’s picture

Hmm OK that'll teach me to try to review patches this early in the morning.

That message needs some information (or a link to more information) on what to do in this case - it doesn't offer any way to fix your database so it can be upgraded.

patrickd’s picture

I'd suggest to do that in a follow up (I'll take care about it personally) so we finally get fixed the critical bug #1969680: Installation fails with MyISAM - key too long

catch’s picture

Status: Reviewed & tested by the community » Needs work
patrickd’s picture

(Still suggesting to discuss this in a followup, we've cross-posted #106)

So the "simplest" fix would be

  • Disable the module
  • Uninstall it (do we actually need to uninstall it, that would suck as the configuration would get lost and the whole point behind doing an upgrade would be for nothing)
  • OR manually search for the module name in the database and replace it with a shorter name
  • rename the module name with a shorter one and search&replace this in all its files
  • Enable it again

I see a lot of potential bike-shedding here and would rather get this committed first to fix our critical bug before starting these discussions

catch’s picture

A link to a stub handbook page would be enough I think - but we should create the handbook page and put the link in the patch. Potentially a site-admin facing change notice?

Contrib modules that are over 50 chars could potentially rename their 7.x versions so that users can upgrade to those first, but that's not going to be the case for custom modules, or for modules that are orphaned in the system table (were never disabled/uninstalled but are still in the filesystem), so there's probably going to be multiple routes out of this situation.

patrickd’s picture

well, not sure where the right place for that handbook page might be
The most obvious place is http://drupal.org/upgrade, but where / which sub-page?
Or should we instead put the upgrade information into the Upgrade.txt?

patrickd’s picture

Assigned: Unassigned » patrickd

needs reroll again

patrickd’s picture

Status: Needs work » Needs review
FileSize
17.33 KB

prior file_update_8002() has been removed, renamed file_update_8003() of this patch to file_update_8002().

jessebeach’s picture

Incremented function system_update_8057() to function system_update_8058()

Patched against: b0cf1be964724303655fa54e92f32014bb4980f4

jessebeach’s picture

Issue summary: View changes

Updated issue summary.

patrickd’s picture

Issue summary: View changes

Updated issue summary

patrickd’s picture

Updated the issue summary.

YesCT suggested to document how to deal with this change on upgrade in an change record, seems to be a good idea for me

patrickd’s picture

Issue summary: View changes

Added super nice ticks and crosses

patrickd’s picture

Create a change record: https://drupal.org/node/2014073

patrickd’s picture

Issue summary: View changes

Fixed grammar

patrickd’s picture

Added a link to the upgrade requirement error to the change record with further information.

Module name too long
@name is @count characters long.
Module names longer than 50 characters are no longer supported.

patrickd’s picture

Issue summary: View changes

Updated remaining tasks

patrickd’s picture

Assigned: patrickd » Unassigned
Priority: Major » Critical

Unassigning from my self.

This is still blocking #1969680: Installation fails with MyISAM - key too long, so raising priority as this is in that case blocking a critical task and a critical bug.

YesCT’s picture

Assigned: Unassigned » YesCT
+++ b/core/includes/bootstrap.incundefined
@@ -191,6 +191,11 @@
+const DRUPAL_EXTENSION_NAME_MAX_LENGTH = 50;

+++ b/core/modules/system/system.installundefined
@@ -2223,6 +2223,25 @@ function system_update_8057() {
+      'length' => 50,

+++ b/core/modules/user/user.installundefined
@@ -1059,5 +1059,43 @@ function user_update_8017() {
+      'length' => 50,

there are many of these lines that use '50'

[edit: #42 looked like it used the const]

make a const?

wait there is one. why not use it?

+++ b/core/includes/update.incundefined
@@ -359,6 +359,18 @@ function update_prepare_d8_bootstrap() {
+            'description' => 'Module names longer than 50 characters are <a href="https://drupal.org/node/2014073">no longer supported</a>.',

this wasn't a t() already, so probably does not need to be now.

what about a format string?

+++ b/core/modules/system/tests/upgrade/drupal-7.module_name_length.database.phpundefined
@@ -0,0 +1,22 @@
+ * @file
+ * Database additions for role tests. Used in
+ * \Drupal\system\Tests\Upgrade\FilledStandardUpgradePathTest.

this needs to be one line summary.

I'll do a patch for the comment length concern.

patrickd’s picture

there are many of these lines that use '50'

make a const?

There already is a constant, but xjm said:
https://drupal.org/node/1852454#comment-7402596

+++ b/core/includes/update.incundefined
@@ -359,6 +359,18 @@ function update_prepare_d8_bootstrap() {

I don't think that bootstrap.inc (and therefore the constant from there) is available here

YesCT’s picture

the doc block for that file was copied.
@tim.plunkett helped me see I could look in the set for the upgrade path tests to see what gz file it relied on, so I could write comments that made sense.

There doesn't seem to be a standard form for the one line @file for these files, so I used something I saw in a couple others in core/modules/system/tests/upgrade
like:
drupal-7.date.database.php
drupal-7.image.database.php

patrickd’s picture

xjm suggested that (while checking the themes name length as requirement is not really possible) we could at least use requirements instead of throwing an exception on the module page.

YesCT’s picture

this may have been more what @catch was asking for in #109.
like noted in #110 I could not find a more appropriate handbook page.
So I did:
https://drupal.org/node/496/revisions/view/2484232/2719039

If someone knows where that info should have gone instead, please post. :)

Turns out, that making the change notice before this is committed is pretty confusing, so that my have not been my best idea.

YesCT’s picture

Assigned: YesCT » Unassigned
YesCT’s picture

Status: Needs review » Needs work

I tried this... by removing the hidden on the test module, and then installing.. and enabling
I didn't get a warning or error. Should I have?

  518  drush am 1852454
  519  git status
  520  git add core*
  521  git status
  522  git commit -m "120"
  523  mvim core/modules/system/tests/modules/invalid_module_name_over_the_maximum_allowed_character_length/invalid_module_name_over_the_maximum_allowed_character_length.info.yml
  524  cat core/modules/system/tests/modules/invalid_module_name_over_the_maximum_allowed_character_length/invalid_module_name_over_the_maximum_allowed_character_length.info.yml

$ cat core/modules/system/tests/modules/invalid_module_name_over_the_maximum_allowed_character_length/invalid_module_name_over_the_maximum_allowed_character_length.info.yml
name: 'Module name length test'
type: module
description: 'Test module with a name over the maximum allowed characters.'
package: Testing
version: VERSION
core: 8.x
hidden: FALSE

  525  echo "manually install"
  526  #but need the db empty
  527  drush -y sql-drop --db-url="mysql://root:root@localhost/drupal" 
  528  drush -y sql-drop --db-url="mysql://root:root@localhost/d8-git" 
  529  echo "had to reboot... forgot to start mamp"
  530  drush -y sql-drop --db-url="mysql://root:root@localhost/d8-git" 
  531  history

http://screencast.com/t/KgbHBJABw104

YesCT’s picture

checked it was actually over
echo "invalid_module_name_over_the_maximum_allowed_character_length" | wc
62

So... I'll try backporting it to 7, installing a 7 site, enabling it and upgrading.

YesCT’s picture

FileSize
223.3 KB

backporting...
https://drupal.org/node/1935708 change record for info to yml

change : to =
(:%s/:/ =/g)
Also change 8.x to 7.x
and hidden true to hidden false

no patch:
part 1 http://screencast.com/t/XeSDXCRystaI
part 2 http://screencast.com/t/1WQclIPeg

----
with patch:
part 1 (install 7, with long module name) http://screencast.com/t/RWn3dZ1ds
part 2 (upgrade) http://screencast.com/t/8ZUwBehn

requirements works to give error about too long name during upgrade, and has link to change record.

link-to-toolong.png

patrickd’s picture

Yay! Thanks for taking all this work to test it manually!
soo.. can we get this back to "needs review" now or did you find a blocking issue with the last patch?

YesCT’s picture

#124 needs confirming...

I found no error when enabling a module in d8 that has a long name. (not upgrading from d7, just plain enabling a long named module in d8.)

We need someone to confirm that (finding the same as me, and meaning it needs work), or to test this and make sure that enabling a module with a long name does show an error (that I must have done something wrong during testing, and that this is back to needs review or rtbc).

patrickd’s picture

Ah, I missed that,

First I tested to enable the stark theme, after renaming it to "stark_much_too_long_name_of_a_theme_this_is_please_fail"
And it could be enabled without any problems

Then I renamed devel module to "devel_much_too_long_name_of_a_module_this_is_please_fail"
And it could be enabled without any problems

So it seems like we need to add some custom validation to both cases here.

Adding remaining task to issue summary.

patrickd’s picture

Issue summary: View changes

Updated remaining tasks

patrickd’s picture

OMG, I'm so dumb, forgot to apply the patch first :D retesting..

chx’s picture

We are not backporting this. There's no way to do it. We should perhaps add a warning to the release notes of the next version of D7 but that's it. We can not break a module in D7 just because it has a long name.

Edit: I might've misunderstood what got backported above. Still, a warning IMO is useful.

patrickd’s picture

@chx, Agreeing, backporting this would be an API break. I think YesCT was just talking about backporting to "too long name test module" to be able to test wether the upgrade path is working.

Again testing like in #129, with patch applied

First I tested to enable the stark theme, after renaming it to "stark_much_too_long_name_of_a_theme_this_is_please_fail"
It failed, as it should:
Drupal\Core\Extension\ExtensionNameLengthException: Theme name <em class="placeholder">stark_much_too_long_name_of_a_theme_this_is_please_fail</em> is over the maximum allowed length of 50 characters

Then I renamed devel module to "devel_much_too_long_name_of_a_module_this_is_please_fail"
No (visible) exception was thrown this time ?!

So we definitely need to add some check and when we're already on it, it can't hurt to add some more helpful error message when enabling a theme.

Therefore leaving remaining tasks as they are

Berdir’s picture

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -546,6 +546,13 @@ public function enable($module_list, $enable_dependencies = TRUE) {
+        // Throw an exception if the module name is too long.
+        if (strlen($module) > DRUPAL_EXTENSION_NAME_MAX_LENGTH) {
+          throw new ExtensionNameLengthException(format_string('Module name %name is over the maximum allowed length of @max characters.', array(
+            '%name' => $module,
+            '@max' => DRUPAL_EXTENSION_NAME_MAX_LENGTH,

Nice find :)

It's not visible from the context here, but this check is within an "if ($enable_dependencies) {", so we only check it if we also check dependencies. But the form submit callback does that separately and passes FALSE as second argument, that's why the test works and the UI not.

So, we need to move this check here out of that if and have a separate loop over the $module_list for this.

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleEnable.phpundefined
@@ -44,4 +45,19 @@ function testRequiredModuleSchemaVersions() {
+    try {
+      module_enable(array($module_name));
+      $this->fail($message);
+    }
+    catch (ExtensionNameLengthException $e) {
+      $this->pass($message);

Should also be possible to confirm in the test by passing FALSE as the second argument. So we should test both, one call with and one without FALSE.

Also, while we're at it, module_enable() should now be $this->container('module_handler')->enable().

YesCT’s picture

Assigned: Unassigned » YesCT

I'll do that.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
17.64 KB

should I do something like:

   public function enable($module_list, $enable_dependencies = TRUE) {
+    foreach ($module_list as $module) {
+      // Throw an exception if the module name is too long.
+      if (strlen($module) > DRUPAL_EXTENSION_NAME_MAX_LENGTH) {
+        throw new ExtensionNameLengthException(format_string('Module name %name is over the maximum allowed len
+          '%name' => $module,
+          '@max' => DRUPAL_EXTENSION_NAME_MAX_LENGTH,
+        )));
+      }
+    }
+
     if ($enable_dependencies) {
       // Get all module data so we can find dependencies and sort.
       $module_data = system_rebuild_module_data();
@@ -546,13 +556,6 @@ public function enable($module_list, $enable_dependencies = TRUE) {
           unset($module_list[$module]);
           continue;
         }
-        // Throw an exception if the module name is too long.
-        if (strlen($module) > DRUPAL_EXTENSION_NAME_MAX_LENGTH) {
-          throw new ExtensionNameLengthException(format_string('Module name %name is over the maximum allowed l
-            '%name' => $module,
-            '@max' => DRUPAL_EXTENSION_NAME_MAX_LENGTH,
-          )));
-        }
         $module_list[$module] = $module_data[$module]->sort;

or...

something like

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -530,7 +530,6 @@ protected function parseDependency($dependency) {
    * {@inheritdoc}
    */
   public function enable($module_list, $enable_dependencies = TRUE) {
-    if ($enable_dependencies) {
       // Get all module data so we can find dependencies and sort.
       $module_data = system_rebuild_module_data();
       // Create an associative array with weights as values.
@@ -555,6 +554,7 @@ public function enable($module_list, $enable_dependencies = TRUE) {
         }
         $module_list[$module] = $module_data[$module]->sort;
 
+        if ($enable_dependencies) {
         // Add dependencies to the list, with a placeholder weight.

but, then might miss if a dependency has a name too long...

so.

I put it after the loop that was adding dependencies.

I think we need a tests only patch to show tests will fail.
I tried commenting out the exception and then running the test locally, and it still passed.
Then I thought, maybe I need to disable the module before the second time of trying to enable it, and @Berdir pointed out that I should not have to do since it should not have been enabled the first time.

fubhy’s picture

The while() loop has to stay as it has to dynamically extend the items it loops over as we keep adding items to the array during the loop.

xjm’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleEnable.phpundefined
@@ -44,4 +45,28 @@ function testRequiredModuleSchemaVersions() {
+      module_enable(array($module_name));
...
+      module_enable(array($module_name), FALSE);

I think we should use $this->container->get('module_handler')->enable() here now? Edit: Or $module_handler = $this->container->get('module_handler'); once and then $module_handler->enable().

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/ExistingModuleNameLengthUpgradePathTest.phpundefined
@@ -0,0 +1,48 @@
+    $this->assertNoFieldByXPath('//input[@type="submit"]', 'Allowed to continue with the update process.');

Why does the absence of a submit button indicate that the user is allowed to continue with the upgrade process? I think the asserion message is backwards?

xjm’s picture

So it seems like we need to add some custom validation to both cases here.

Adding remaining task to issue summary.

For themes, we'd need to validate, but for modules, can we display the requirements not met warning on the module page like for other unmet requirements?

YesCT’s picture

this is just adding what I think should have been a test showing that the previous patch (120) should have failed, since it does, per @Berdir in #133

Should also be possible to confirm in the test by passing FALSE as the second argument. So we should test both, one call with and one without FALSE.

If this fails, that is good, it means the test would have picked up that module with the long name was able to be enabled (did not throw an exception).
If this fails, then we can add back in the fix of moving the test for the module name out of the depencencies if and into it's own loop as in #134, and then the next step will be to update it since:
module_enable() should now be $this->container('module_handler')->enable()

If this passes, then we need a different test that would work to show the patch in 120 should have failed.

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-139-shouldfail-addsUItest-noUIfix.patch, failed testing.

Berdir’s picture

Failed as expected, let's address #137 and get this in.

Which means using $this->container and fix the assertion message. It's the same check as the one that UpgradePathTestBase has, I just added the No and forgot to update the assertion message. So let's just add a Not in front of that.

Berdir’s picture

As to requirements handling, while nice, I think that's a nice to have and not a critical task like this issue and the two criticals that are blocked on this. It's a very rare and very developer-facing error, I'm fine with an exception here.

Requirements might need quite a bit of changes because they are called in different places and right now only the module itself can define them, so we might have to change multiple places to make this work.

YesCT’s picture

Status: Needs work » Needs review
FileSize
4.07 KB
18.2 KB

this uses
$this->container('module_handler')->enable
instead of
module_enable()
just in the lines this patch was touching.

It also updates the text on the assert message

It makes the doc block and the description for the test more accurate/generic.

-      'description' => 'Tests module_enable().',
+      'description' => 'Tests enabling modules.',

And it puts the check for long names after, outside, the dependency loop so that enables from the UI are also caught.

I agree lets address the nice requirements somewhere else. #2020939: Add a requirements check for the 50 character restricted module and theme name lengths

YesCT’s picture

Issue summary: View changes

Updated remaining tasks

Status: Needs review » Needs work

The last submitted patch, module-name-1852454-143.patch, failed testing.

xjm’s picture

I guess a followup is OK, but the approach was recommended back in January.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleEnable.phpundefined
@@ -52,7 +52,7 @@ function testModuleNameLength() {
-      module_enable(array($module_name));
+      $this->container('module_handler')->enable(array($module_name));

$this->container->get('module_handler'), container is a property of the class, not a method.

xjm’s picture

@Berdir, that was your typo; see #133. ;) (I put the correct form in #137.)

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
FileSize
1.39 KB
18.21 KB

/me repeats () works with methods, like get, not properties

here is just that change.

I dont want to hold up this issue, unassigning.

Module enable tests passing locally.

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

Very nice. This looks ready. :)

catch’s picture

Title: Restrict module and theme name lengths to 50 characters » Change notice: Restrict module and theme name lengths to 50 characters
Status: Reviewed & tested by the community » Active

Looks ready to me too.

Committed/pushed to 8.x, thanks!

Will need a short change notice.

patrickd’s picture

Title: Change notice: Restrict module and theme name lengths to 50 characters » Restrict module and theme name lengths to 50 characters
Status: Active » Fixed

@catch, we already have: https://drupal.org/node/2014073 :-)

Thanks to everyone who helped with this issue! This fixes a huge problem for simplytest.me!

patrickd’s picture

Issue summary: View changes

updated tasks. -y

Gábor Hojtsy’s picture

@patrickd: woot! Clean simplytest.me FTW!

xjm’s picture

Hooray! Awesome to see this in finally. It solves so many problems! Thanks everyone!

Wim Leers’s picture

Great job, @patrickd et al :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

updated

xjm’s picture

Issue tags: +Naming things is hard
markusa’s picture

This would be something nice to backport to D7. Just spent hours figuring out how a custom module I wrote broke all my admin pages, and it was the length of the module name (49 characters works)