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.

Files: 
CommentFileSizeAuthor
#148 module-name-1852454-148.patch18.21 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,670 pass(es).
[ View ]
#148 interdiff-143-148.txt1.39 KBYesCT
#143 module-name-1852454-143.patch18.2 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,855 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#143 interdiff-139-143.txt4.07 KBYesCT
#139 module-name-1852454-139-shouldfail-addsUItest-noUIfix.patch17.81 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,707 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#139 interdiff-120-139-fail.txt908 bytesYesCT
#135 module-name-1852454-135.patch17.64 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,610 pass(es).
[ View ]
#135 interdiff-120-134.txt2.31 KBYesCT
#126 link-to-toolong.png223.3 KBYesCT
#120 module-name-1852454-120.patch17.39 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 55,525 pass(es).
[ View ]
#120 interdiff-116-120.txt1 KBYesCT
#116 module-name-1852454-116.patch17.51 KBpatrickd
PASSED: [[SimpleTest]]: [MySQL] 55,084 pass(es).
[ View ]
#116 interdiff.txt1.18 KBpatrickd
#113 module-name-1852454-113.patch19.55 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 57,834 pass(es), 4 fail(s), and 6 exception(s).
[ View ]
#112 module-name-1852454-112.patch17.33 KBpatrickd
PASSED: [[SimpleTest]]: [MySQL] 55,367 pass(es).
[ View ]
#99 module-name-1852454-99.patch17.28 KBpatrickd
PASSED: [[SimpleTest]]: [MySQL] 57,821 pass(es).
[ View ]
#99 interdiff.txt1.2 KBpatrickd
#98 diff-90-97.txt7.51 KBpatrickd
#97 module-name-1852454-97.patch17.17 KBdjroshi
PASSED: [[SimpleTest]]: [MySQL] 57,649 pass(es).
[ View ]
#96 module-name-1852454-96.patch17.96 KBdjroshi
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.install.
[ View ]
#95 module-name-1852454-95.patch17.46 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch module-name-1852454-95.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#94 module-name-1852454-94.patch17.47 KBpatrickd
PASSED: [[SimpleTest]]: [MySQL] 55,798 pass(es).
[ View ]
#91 module-name-1852454-90.patch17.53 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,723 pass(es).
[ View ]
#91 module-name-1852454-90-interdiff.txt5.89 KBBerdir
#90 w3Nbr.gif1.67 MBchx
#84 module-name-1852454-84.patch11.63 KBpatrickd
PASSED: [[SimpleTest]]: [MySQL] 56,538 pass(es).
[ View ]
#84 module-name-1852454-84-interdiff.txt1.43 KBpatrickd
#83 module-name-1852454-83.patch11.64 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 56,414 pass(es).
[ View ]
#83 module-name-1852454-83-interdiff.txt3.11 KBBerdir
#81 module-name-1852454-81.patch9.59 KBPancho
FAILED: [[SimpleTest]]: [MySQL] 55,827 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#72 module-name-1852454-72.patch11.64 KBPancho
FAILED: [[SimpleTest]]: [MySQL] 55,448 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#70 module-name-1852454-69.patch11.06 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 55,596 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#69 module-name-interdiff.txt5.11 KBxjm
#67 module-name-1852454-67.patch12.51 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 55,444 pass(es), 3 fail(s), and 10 exception(s).
[ View ]
#67 lol.txt223 bytesxjm
#65 module-name-1852454-65.patch12.55 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#65 interdiff.txt486 bytesxjm
#62 module-name-1852454-62.patch12.5 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.install.
[ View ]
#62 interdiff-module-name.txt10.09 KBxjm
#52 module-name-1852454-52.patch11.87 KBpatrickd
FAILED: [[SimpleTest]]: [MySQL] 55,592 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#48 module-name-1852454-48.patch12.49 KBdealancer
PASSED: [[SimpleTest]]: [MySQL] 55,866 pass(es).
[ View ]
#46 module-name-1852454-46.patch10.43 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,663 pass(es).
[ View ]
#46 module-name-1852454-46-interdiff.txt1.5 KBBerdir
#44 module-name-1852454-44.patch9.8 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 55,426 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#44 interdiff.txt1.31 KBamateescu
#42 module-name-1852454-42.patch9.63 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 54,805 pass(es), 31 fail(s), and 30 exception(s).
[ View ]
#42 interdiff.txt7.03 KBamateescu
#40 module-name-1852454-40-pass.patch9.84 KBpatrickd
FAILED: [[SimpleTest]]: [MySQL] 55,365 pass(es), 31 fail(s), and 30 exception(s).
[ View ]
#38 interdiff.txt2.17 KBpatrickd
#38 module-name-1852454-38-pass.patch6.69 KBpatrickd
FAILED: [[SimpleTest]]: [MySQL] 55,818 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#32 module-name-1852454-32-fail.patch2.44 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 50,810 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#32 module-name-1852454-32-pass.patch4.52 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,687 pass(es).
[ View ]
#30 module-name-1852454-30-tests.patch999 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] 50,787 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#30 module-name-1852454-30.patch3.06 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 50,780 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#23 module-name-1852454-23.patch2.08 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,661 pass(es).
[ View ]

Comments

Great! don't forget about db-schema some DBs has limit on table names and views hits this limit for aliases #571548: truncated identifiers with more than 63 chars, causing Views to break

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

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

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

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

#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?

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Title:Define a max module name lengthDoes 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.

Issue summary:View changes

Updated issue summary.

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.

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.

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

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.

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

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

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

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.

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.

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.

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

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

Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new2.08 KB
PASSED: [[SimpleTest]]: [MySQL] 50,661 pass(es).
[ View ]

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.06 KB
FAILED: [[SimpleTest]]: [MySQL] 50,780 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new999 bytes
FAILED: [[SimpleTest]]: [MySQL] 50,787 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new4.52 KB
PASSED: [[SimpleTest]]: [MySQL] 50,687 pass(es).
[ View ]
new2.44 KB
FAILED: [[SimpleTest]]: [MySQL] 50,810 pass(es), 4 fail(s), and 2 exception(s).
[ View ]

Oops, #30 is incomplete.

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

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.

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

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

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.

StatusFileSize
new6.69 KB
FAILED: [[SimpleTest]]: [MySQL] 55,818 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.17 KB

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

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?

Status:Needs work» Needs review
StatusFileSize
new9.84 KB
FAILED: [[SimpleTest]]: [MySQL] 55,365 pass(es), 31 fail(s), and 30 exception(s).
[ View ]

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.

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?

Status:Needs work» Needs review
StatusFileSize
new7.03 KB
new9.63 KB
FAILED: [[SimpleTest]]: [MySQL] 54,805 pass(es), 31 fail(s), and 30 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
new9.8 KB
FAILED: [[SimpleTest]]: [MySQL] 55,426 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new1.5 KB
new10.43 KB
PASSED: [[SimpleTest]]: [MySQL] 55,663 pass(es).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new12.49 KB
PASSED: [[SimpleTest]]: [MySQL] 55,866 pass(es).
[ View ]

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

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

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

Yeah, sure, remove that thing :)

Status:Needs work» Needs review
StatusFileSize
new11.87 KB
FAILED: [[SimpleTest]]: [MySQL] 55,592 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Removed the SchemaIndexLengthException class file

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

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

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

Issue summary:View changes

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

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?

How is it not fatal?

Title:Restrict module and theme name lengths to 40 charactersRestrict 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

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.

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.

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.

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.

StatusFileSize
new10.09 KB
new12.5 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.install.
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new486 bytes
new12.55 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new223 bytes
new12.51 KB
FAILED: [[SimpleTest]]: [MySQL] 55,444 pass(es), 3 fail(s), and 10 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.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...

StatusFileSize
new11.06 KB
FAILED: [[SimpleTest]]: [MySQL] 55,596 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new11.64 KB
FAILED: [[SimpleTest]]: [MySQL] 55,448 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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: UTF-8: support 4 bytes characters in MySQL.

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.

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

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

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

@@ -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. :)

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

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.

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

Thanks!

Status:Needs work» Needs review
StatusFileSize
new9.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,827 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Assigned:xjm» Unassigned
Status:Needs work» Needs review
StatusFileSize
new3.11 KB
new11.64 KB
PASSED: [[SimpleTest]]: [MySQL] 56,414 pass(es).
[ View ]

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.

StatusFileSize
new1.43 KB
new11.63 KB
PASSED: [[SimpleTest]]: [MySQL] 56,538 pass(es).
[ View ]

Small coding standard changes..

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

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.

Assigned:Unassigned» patrickd

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?

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.

Assigned:patrickd» Unassigned

This sounds like a case for chx?^^

StatusFileSize
new1.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.

StatusFileSize
new5.89 KB
new17.53 KB
PASSED: [[SimpleTest]]: [MySQL] 55,723 pass(es).
[ View ]

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

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?

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 :)

Issue tags:-Needs upgrade path tests
StatusFileSize
new17.47 KB
PASSED: [[SimpleTest]]: [MySQL] 55,798 pass(es).
[ View ]

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? :)

StatusFileSize
new17.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch module-name-1852454-95.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new17.96 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.install.
[ View ]

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

StatusFileSize
new17.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,649 pass(es).
[ View ]

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

StatusFileSize
new7.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?

StatusFileSize
new1.2 KB
new17.28 KB
PASSED: [[SimpleTest]]: [MySQL] 57,821 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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 :)

@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!

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.

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 ?

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.

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

Status:Reviewed & tested by the community» Needs work

(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

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.

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?

Assigned:Unassigned» patrickd

needs reroll again

Status:Needs work» Needs review
StatusFileSize
new17.33 KB
PASSED: [[SimpleTest]]: [MySQL] 55,367 pass(es).
[ View ]

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

StatusFileSize
new19.55 KB
FAILED: [[SimpleTest]]: [MySQL] 57,834 pass(es), 4 fail(s), and 6 exception(s).
[ View ]

Incremented function system_update_8057() to function system_update_8058()

Patched against: b0cf1be964724303655fa54e92f32014bb4980f4

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary

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

Issue summary:View changes

Added super nice ticks and crosses

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

Issue summary:View changes

Fixed grammar

StatusFileSize
new1.18 KB
new17.51 KB
PASSED: [[SimpleTest]]: [MySQL] 55,084 pass(es).
[ View ]

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.

Issue summary:View changes

Updated remaining tasks

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.

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.

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

StatusFileSize
new1 KB
new17.39 KB
PASSED: [[SimpleTest]]: [MySQL] 55,525 pass(es).
[ View ]

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

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.

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.

Assigned:YesCT» Unassigned

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

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.

StatusFileSize
new223.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

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?

#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).

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.

Issue summary:View changes

Updated remaining tasks

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

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.

@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

+++ 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().

Assigned:Unassigned» YesCT

I'll do that.

Status:Needs work» Needs review
StatusFileSize
new2.31 KB
new17.64 KB
PASSED: [[SimpleTest]]: [MySQL] 57,610 pass(es).
[ View ]

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.

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.

+++ 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?

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?

StatusFileSize
new908 bytes
new17.81 KB
FAILED: [[SimpleTest]]: [MySQL] 57,707 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
new18.2 KB
FAILED: [[SimpleTest]]: [MySQL] 57,855 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Issue summary:View changes

Updated remaining tasks

Status:Needs review» Needs work

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

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

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

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

Assigned:YesCT» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.39 KB
new18.21 KB
PASSED: [[SimpleTest]]: [MySQL] 57,670 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Very nice. This looks ready. :)

Title:Restrict module and theme name lengths to 50 charactersChange 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.

Title:Change notice: Restrict module and theme name lengths to 50 charactersRestrict 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!

Issue summary:View changes

updated tasks. -y

@patrickd: woot! Clean simplytest.me FTW!

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

Great job, @patrickd et al :)

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

updated

Issue tags:+Naming things is hard