Length of menu_tree.url and menu_tree.route_param_key are too short (255 characters)

Created on 14 January 2020, over 5 years ago
Updated 11 February 2023, over 2 years ago

Problem/Motivation

URL/route lengths in the database need to be long enough for typical use. In reviewing the codebase, we found two menu_tree database fields (route_param_key and url) that are too short (255 characters). This can cause errors when adding data.

Steps to reproduce

To reproduce the url limitation:

1. Go to: admin/structure/menu/manage/[menuname]/add
2. Enter a very long external URL (longer than 255 characters)
3. You will get an error:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'url' at row 1

Proposed resolution

Change from 255 to 2048 in the schema and in an update hook.

Remaining tasks

User interface changes

API changes

Data model changes

The size of the menu_tree database fields (route_param_key and url) are increased.

Release notes snippet

Original report by [username]

At admin/structure/menu/manage/MENU_NAME/add, long, external URL's result in

Drupal\Core\Entity\EntityStorageException: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'url' at row 1

The database column is only 255, set at core/lib/Drupal/Core/Menu/MenuTreeStorage.php in MenuTreeStorage::schemaDefinition()
Attached is a simple patch. Any reason not to up it?

πŸ› Bug report
Status

Needs work

Version

10.1 ✨

Component
Menu systemΒ  β†’

Last updated about 17 hours ago

Created by

πŸ‡ΊπŸ‡ΈUnited States jacob.embree

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Patch #36 (for 9.5) was a clean re-roll of #28 (for 9.4) except that the hook update was renamed from system_update_9101() to system_update_9500(). There was also a new patch #36 for 10.1.x which has system_update_10101() but this no longer applies in 10.1.x. The current 10.1.x. system.install only has one hook update:

    /**
     * Implements hook_update_last_removed().
     */
    function system_update_last_removed() {
      return 8805;
    }
    
    /**
     * Update the stored schema data for entity identifier fields.
     */
    function system_update_8901() 
    

    So what should the new update be called in 10.1.x? Is system_update_10101() correct? I have converted patch 36 to a MR and left it at 10101 but I do not know if this is correct.

    From @larowlan in #34

    Also, I'm not sure its a bug, more-so a feature or task, and that influences my thinking about where we'd commit it.

    I would say it is a bug, because generating content via the Devel module can fail with this error, see #3027826: String data, right truncated: 1406 Data too long for column 'menu_name' β†’ and the follow-up on https://gitlab.com/drupalspoons/devel/-/issues/212

  • @jonathan1055 opened merge request.
  • @jonathan1055 opened merge request.
  • πŸ‡ΊπŸ‡ΈUnited States edmund.dunn Olympia, WA

    Posting the static patch because using the MR doesn't allow pinning to a specific commit, so anyone can submit pretty much anything and inject it into our codebase IIRC. This also fixed our issue.

  • last update about 2 years ago
    Custom Commands Failed
  • last update about 2 years ago
    29,439 pass
  • πŸ‡§πŸ‡ͺBelgium timcamps

    Thank you, was facing the same problem when linking to a facet page.
    The patch in #46 fixes the issue on Drupal 10.1.6.

  • πŸ‡ΊπŸ‡ΈUnited States edmund.dunn Olympia, WA

    I am rerolling for 10.2.

  • Status changed to Needs review over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States edmund.dunn Olympia, WA
  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    As a bug will need test coverage.

    Update hook will require a test as well.

    May need submaintainer sign off.

  • πŸ‡΅πŸ‡±Poland adpo

    Hello, I am experiencing the same issue as described in #47 (link to views with facets), however, the link is short: "products/school-type/2," while the that path has a long route_param_key:

    [:db_update_placeholder_28] => f0=&f1=&f2=&f3=&f4=&f5=&f6=&f7=&f8=&f9=&f10=&f11=&f12=&f13=&f14=&f15=&f16=&f17=&f18=&f19=&f20=&f21=&f22=&f23=&f24=&f25=&f26=&f27=&f28=&f29=&f30=&f31=&f32=&f33=&f34=&f35=&f36=&f37=&f38=&f39=&view_id=product_catalog&display_id=product_catalog&facets_query=school-type/primary-2 [:db_condition_placeholder_0] => 500 ) in Drupal\Core\Menu\MenuTreeStorage->doSave() (line 303 of core/lib/Drupal/Core/Menu/MenuTreeStorage.php)
    

    I have decided to use external link http://mywebsite.com/products/school-type/2 for my internal path to resolve that issue, however, this is not an ideal solution.

  • πŸ‡ΊπŸ‡ΈUnited States jrglasgow Idaho

    jrglasgow β†’ changed the visibility of the branch 3106205-menutree-url-and-route-length to hidden.

  • πŸ‡ΊπŸ‡ΈUnited States jrglasgow Idaho

    I created a new merge request to 11.x and changed the system_update hook to 10202 so it ensured to run.

  • Pipeline finished with Success
    over 1 year ago
    Total: 541s
    #96069
  • πŸ‡§πŸ‡·Brazil murilohp

    Just uploading an static patch from the MR, after upgrading the core to 10.2, the hook update conflicted with an older one. If anyone else is having the same issue, I also had to update the hook update number in order to ensure the hooks will be executed.

    FYI: drush ev "\Drupal::keyValue('system.schema')->set('system', 10100)";

  • πŸ‡ΊπŸ‡ΈUnited States brad.bulger

    fwiw, we have a few dozen domains each with their own menu on our site, and we had to change route_param_key to a longblob - some values are over 10K.

  • First commit to issue fork.
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 165s
    #221962
  • Pipeline finished with Failed
    about 1 year ago
    #221964
  • Pipeline finished with Failed
    about 1 year ago
    Total: 332s
    #221986
  • Pipeline finished with Success
    about 1 year ago
    Total: 549s
    #221991
  • Status changed to Needs review about 1 year ago
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    Code updated against 11.x and upgrade path test coverage added.

  • Pipeline finished with Canceled
    12 months ago
    Total: 101s
    #244765
  • Pipeline finished with Success
    12 months ago
    Total: 468s
    #244766
  • Status changed to RTBC 12 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Applied some nitpicky typehints and returns

    Ran test-only feature, https://git.drupalcode.org/issue/drupal-3106205/-/jobs/2347336 which shows coverage

    Applied locally and update hook ran fine.

    Believe this is good.

  • Status changed to Needs work 12 months ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Left comments in the MR. This will need a change record. And this one for a similar change, https://www.drupal.org/node/3227494 β†’

  • πŸ‡¬πŸ‡§United Kingdom jamiehollern Alba/Scotland

    Re-roll for 10.3.

  • Pipeline finished with Failed
    10 months ago
    Total: 500s
    #303767
  • First commit to issue fork.
  • heddn Nicaragua

    This is ready for another review again.

  • Pipeline finished with Failed
    10 months ago
    Total: 333s
    #304458
  • Pipeline finished with Failed
    10 months ago
    Total: 1043s
    #304473
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    There are multiple branches open for this ticket. Could the ones not in use be hidden.

    believe the test failure is random, but was tagged for a change record which still appears needed.

  • πŸ‡§πŸ‡·Brazil murilohp

    murilohp β†’ changed the visibility of the branch 10.2.x to hidden.

  • πŸ‡§πŸ‡·Brazil murilohp

    murilohp β†’ changed the visibility of the branch 10.1.x to hidden.

  • πŸ‡§πŸ‡·Brazil murilohp

    Just added a CR for this issue, and hide the unnecessary branches. Moving back to NR

  • πŸ‡ΊπŸ‡ΈUnited States pwolanin

    Took a look at the 11.x MR. Looks like a simple and sensible fix, so moving back to RTBC.

    • alexpott β†’ committed 5b7419e8 on 11.1.x
      Issue #3106205 by jrglasgow, plopesc, jonathan1055, smustgrave, edmund....
    • alexpott β†’ committed 35044e45 on 11.x
      Issue #3106205 by jrglasgow, plopesc, jonathan1055, smustgrave, edmund....
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Committed and pushed 35044e4519e to 11.x and 5b7419e8dfa to 11.1.x. Thanks!

    FWIW we could backport this to 10.4.x / 10.5.x using https://www.drupal.org/node/3459876 β†’

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Just checking, will this not be committed to 11.0.x ?

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

  • πŸ‡ΊπŸ‡ΈUnited States amanire

    Well, this issue is closed, but I would really like to get this working in 10.3.10 and I don't want to have any phantom core update hooks lying around. I considered creating a local patch for the schema definition and manually setting the length in the DB but that seems unwise.

    So given the timing, I'll settle for a 10.4 release. I'm not really sure how to proceed with a fork in this scenario so I regenerated a patch using the API suggestion in #71. I think it will also require patching core 11.x (and 11.1.x?) with this:

    $equivalent_update = \Drupal::service('update.update_hook_registry')->getEquivalentUpdate();
      if ($equivalent_update instanceof \Drupal\Core\Update\EquivalentUpdate) {
        return $equivalent_update->toSkipMessage();
      }

    Is this patch helpful or do we need a core maintainer/git ninja to backport system.install?

  • Status changed to Fixed 6 months ago
  • πŸ‡΅πŸ‡±Poland adpo

    For the quick sulution for exisitng website, can I just run ALTER TABLE menu_tree MODIFY route_param_key VARCHAR(1024);
    Do you see any drawbacks in such a solution?

  • πŸ‡ΊπŸ‡ΈUnited States amanire

    @adpo the reason that I'm not doing that is because it will cause a mismatch with the schema definition in /core/lib/Drupal/Core/Menu/MenuTreeStorage.php. That may lead to unexpected issues with Schema API that I'm not willing to risk.

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    .

  • πŸ‡§πŸ‡ͺBelgium ludo.r Brussels

    This is a re-roll for 10.4.x (update hook number needs to be updated).

  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    I am still on Drupal 10.5.1 with Facets, and as others also get an "1406 Data too long for column 'route_param_key'" error from a string like this:

    facets_query=&f0=&f1=&f2=&f3=&f4=&f5=&f6=&f7=&f8=&f9=&f10=&f11=&f12=&f13=&f14=&f15=&f16=&f17=&f18=&f19=&f20=&f21=&f22=&f23=&f24=&f25=&f26=&f27=&f28=&f29=&f30=&f31=&f32=&f33=&f34=&f35=&f36=&f37=&f38=&f39=&view_id=browse_motives_search_api&display_id=motives.

    Is a Drupal 10 backport worth considering, or is the recommendation to upgrade to Drupal 11?

  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    I think it would be really helpful, if someone who knew the details could share the steps to fix this in Drupal 10. Perhaps something along these lines, under a new "Workaround" header?

    Workaround for Drupal 10

    You can patch Drupal 10 with these steps:

    1. Download the latest re-roll for Drupal 10.5 from comment #XYZ
    2. Set the update hook number to (what?)
Production build 0.71.5 2024