MenuLinkContent menu_name max length is too long

New MR for 10.1.x - just one commit, the patch in #13 by YurkinPark. The tests failed before so will fail again.

Since the MR has so many failures this still needs some works

Also think there should be a test case and possibly an upgrade path

... possibly an upgrade path

Yes that is the single thing that is causing the test failures. They all have

Custom menu link: The Menu name field needs to be updated.

which is the outcome of running UpdatePathTestTrait runUpdates(), which is called from many tests. I think is due to there being no hook_update to set the loaded field definition to match the updated definition in code. When viewing the status page we get the usual

The following changes were detected in the entity type and field definitions.
Custom menu link - The Menu name field needs to be updated.

I have been trying to write a hook_update to fix this but have not got it right yet. I can detect the problem but saving the field definition does not seem to make any effect. I could share what I've got so far.

I've added the hook_update, took a while to discover how to get the schema updated. But this works locally in update.php and also in phpunit tests.

Existing tests all pass now. Reading the issue summary, the hook update needs to set a state value if any existing menu_name is longer than 32. But it appears that this is an edge-case from reading comment #6

The only ones affected are those who made a custom tree storage backend to allow for longer than 32 characters but have not yet used this capability

I will add that, and it will inform how we write a test for this (if one really is needed)

Looks good @jonathan1055!

Moving to NW for the tests

There are two questions here. , regarding a test for this, what do we actually want to test?

1. Some tests check that a hook_update does what it is meant to do, but those are often when actual storage or entities are modified. All we are doing in this update is re-aligning the schema with the field definition.
2. Or do we want to test that a menu cannot be added with a name longer than 32? Menu names already cannot be added with length longer than 32 via core UI. As mentioned in #6 the only way this could have been done is by writing a custom tree storage backend to allow for longer than 32 characters. I don't see how that could be feasable to be tested.
3. Or do we want to check createWithSampleValues() no longer fails? For that we have the issue #3087634: Adds tests of createWithSampleValues → which is already set up to cover all fields, not just menu_name

, in the update hook, the maximum length of existing menus can be found, but what exactly should be done with the answer? The original issue summary said 'store a state value' and 'create an exception in D10' but is that really the best thing? Could the hook_update either fail or give a warning instead? From what I understand, this change is aligning the field definition max length with what was already set to 32 in the menu storage definition. Any existing menu longer than 32 can still remain and will function fine without any need to be changed.

Happy to continue on this, but I need some discussion and help with the above two questions. I came to fix this issue because I want to help the Devel users who have this problem, see #3027826: String data, right truncated: 1406 Data too long for column 'menu_name' → and the update on drupalspoons/devel issue 212

@jonathan1055

I think we will want option 1 for sure.

If 2 can't be achieved via core we may not need to test.

3 can be covered in the other ticket I think.

For your second question will have to defer to someone else.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Here is a patch that can be used via composer.json.

From the discussion on #27 and #28, I've implemented following changes.

1. Added a test case to ensure that update hook is checking and updating state variables
2. Added another test case with a scenario which covers a custom module that overrides the length of the menu name.
Our service decorator is implementing core service and increasing a menu_name length. I've implemented this scenario as well.

3. I think we might not need to test this as part of this ticket. Happy to explore ways if someone has other opinions.

Patch is significantly larger than the one in #30 because of new test module.

Do we need a CR for this one? Since it is just a minor change in the length of the menu name.

I don't see a BC problem with custom modules overriding length as it has been covered in my test case patch.

Fixing code formatting issues and changing update hook number.

Removing the Needs tests tag as they appear to be added.

Tried running the update hook on 11.x though and got

>  [error]  TypeError: ArrayObject::__construct(): Argument #1 ($array) must be of type array, bool given in ArrayObject->__construct() (line 15 of /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/AbstractListData.php) #0 /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/AbstractListData.php(15): ArrayObject->__construct(false)
> #1 /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/UnstructuredListData.php(21): Consolidation\OutputFormatters\StructuredData\AbstractListData->__construct(false)
> #2 /var/www/html/vendor/drush/drush/src/Commands/core/UpdateDBCommands.php(132): Consolidation\OutputFormatters\StructuredData\UnstructuredListData->__construct(false)

May be unrelated but I can't update to give a review.

Will leave in review if anyone else is able.

Okay I had to upgrade to drush12 to run but I was able to run.

I'm 99% sure the issue is related to drush and not this patch.

So the update hook ran fine and the changes seem simple enough.

Not sure if a change record will be needed but will let committer decide.

@darrenoh what are you changing this was previously RTBC but will have to go through review again.

There was a merge request for 9.3.x. No chance that would get accepted.

#33 the last the last patch uploaded.

Hid the other patches. Leaving merge requests for those who need this fix for D9 and D10.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Looks like PR !4197 and !4181 seems to be missing the test cases added in #43
Adding test cases again in the patch.

Patch was not getting applied cleanly to 11.x. So re-rolled that.
Re-roll diff is against #43 only.

Thanks @mohit_aghera!

One of the proposed solution items is

Determine how and when to trigger a deprecation error

Is that still needed?

Definitely think a change record will be needed

Re-rolling for 9.5.11. The system schema version gets updated to 1011 with the current patches which could mess things up when it comes time to upgrade to Drupal 10.

What on earth is going on in that MR?

The patch from #44 seems to be the the most recent that also applies to HEAD. Will give that one a go and report back.

Re-roll patch from #44 to be compatible with changes from issue #2869568

I am working on a site that uses layout builder, and the preview on the manage layout page runs createWithSampleValues which does result in the database error:
String data, right truncated: 1406 Data too long for column 'menu_name'

I applied patch 48 and the error went away. Drupal 10, PHP 8.1