Shortcut links without a title cause deprecation notices

I've come across this as well, caused and resolved as per @loze's description. Setting back to NR assuming that's answered @larowlan's questions.

Think we need to confirm this is happening without seven theme.

May want to consider adding a default, even if it's an empty string.

I will be working on this during DrupalCon 2023 Pittsburgh.

Just to be sure I tried to reproduce the problem, instructions on comment #10 were very helpful. I created a local environment with Docksal running PHP 8.1.8 and Drupal 9.5.9.

I wasn't able to get the error myself. Tried a bit more than just changing the theme to seven, but couldn't either. Even made sure I was going through the code/function sort() by adding some logging. I see the logged message, but not the error mentioned.

My take would be similar to what I see in comment #13, I get the impression the problem is more being empty than anything else. In any case the patch seems to be harmful, small and to the point. Even follows a similar tactic which is used for weight. But I'm not sure if it is really needed and for what I know from the community we don't like to push stuff if it is 100% needed (which doesn't seem the case).

We just found this issue when we tried to add a Shortcut to a page that is generated via routeing + controller + custom theming.
E.g something like this in a modules routing (generalized path etc)

foo.bar:
  path: '/foo/thing/bar/{id}'
  defaults:
    _controller: '\Drupal\foo\Controller\fooController::method'
    _title: 'Foo thing bar'
  requirements:
    _role: 'administrator'
  options:
    no_cache: 'TRUE'

This might be useful for reproducing so thought I should add this.

I agree with @ibonelli in #16. The issue occurs when displaying or manipulating shortcuts that do not have their title set. This should not be possible, since 'title' is a required field on the Shortcut entity. However it is currently possible to create shortcut entities with NULL as title. The proposed solution hides the issue but does not tackle the root cause (there should not be any shortcuts with NULL as title.).

The easiest way to reproduce the bug is using the steps outlined by @loze in #10 (creating a page display on a view without a title, and then clicking the shortcut quick add link on that page), but even without Views the issue can occur on any page without a title. It does seem limited to Shortcuts added via the "quick add"-link, as the "formal" interface for adding shortcuts has the title field as a required field.

Test-only patch attached that triggers the deprecation notice.

I see a couple of options to resolve this issue:

1. Hide the "quick add link" on pages without a title. It does not make sense to add these pages as shortcuts, since without a title the link(s) don't seem to make sense to end users. This would be confusing to end-users though (why can't I add a shortcut to this page?)
2. Show the "quick add link" on those pages, but instead redirect it to the shortcut add form so users can provide a title manually.
3. Provide a default when adding a shortcut to pages without a title, for example a blank string or "(empty)". Write an update hook to replace the NULLs with the chosen default (easy if we go with a blank string, might be trickier when we choose a translatable string).

To fully address the issue, we will also need an update hook.

Setting to NR for the testbot.

Fixed code style issues.

The last submitted patch, 19: 3280848-2-test-only.patch, failed testing. View results →

Updating for 11.x

Took the tests from #19 and just added a default return in getTitle()

Regards to #18,

Hide the "quick add link" on pages without a title. It does not make sense to add these pages as shortcuts, since without a title the link(s) don't seem to make sense to end users. This would be confusing to end-users though (why can't I add a shortcut to this page?)

Not opposed to removing but seems like a follow up to discuss if we should remove. As that will probably need a usability review

Show the "quick add link" on those pages, but instead redirect it to the shortcut add form so users can provide a title manually.

Believe you meant if #1 didn't work lets do #2. Which I do like this option, but would say same follow up ticket as above would be the place, as usability could go back n forth.

Provide a default when adding a shortcut to pages without a title, for example a blank string or "(empty)". Write an update hook to replace the NULLs with the chosen default (easy if we go with a blank string, might be trickier when we choose a translatable string).

This may be the correct solution here maybe (will let the community vote). Essentially you're saying instead of checing in the getTitle() you want to do a check in the setTitle() before save.

@larowlan as a framework manager what are your thoughts on this too?

The last submitted patch, 21: 3280848-21.patch, failed testing. View results →

So this fix makes me question everything I said in #22 about follow ups. Thoughts though before any more work is done?

Leaving to needs review for other opinions, but the patch in #24 only addresses shortcuts created after applying the patch. Since shortcuts without titles are still stored in the database, the deprecation notices will still appear on the shortcut management page. I think a complete solution either requires an update hook, or hiding the issue (as was done in the initial patches in this thread).

Using '(empty)' is IMO an elegant solution, but should we account for languages other than English?

You are correct.

Also should update the test to show the text for empty.

May be worth checking if empty is an already translated word or if a new word should be used.

Added post_update hook with test.

I looked at the code for a bit and to me it looks okay. The one thing I'm not 100% convinced of is the part where the translation is fetched to be saved into the shortcut entity. I could be wrong, but that should be tested either manually or with a unit test.

@BramDriesen tested that out

Enabled a 2nd language (arabic)
Enabled shortcut links to be translated
Added a shortcut without a title, used the test module in this MR.
Translated the shortcut link to "blah"
Went to the ar/ url and the shortcut correctly showed blah

I was more concerned about the actual translation of (empty).

E.g. You have the word "empty" in multiple languages on your site. You already have a shortcut in your system in E.g arabic without a title. The post update hook comes in. In what language is "empty" added to the shortcut? Is it the default language, the language you came from when hitting updb or is it the actual language from the entity.

Ah so I did another test

Started over with fresh DB
Enabled 2nd language
Enabled shortcut links to be translated
Translated empty to "test"
Using the test module added a shortcut with no title
It shows "test"
Translated shortcut link and changed to "test2"
shortcut link now shows test2

That address the use case?

Yes I think so :) it's an edge case anyway.

Feel free to resolve my comment in the MR.

It sounds like the merge request is canonical here. Please remember to hide patch files when you convert to a merge request.

xjm → closed merge request !2285

I also closed the year-old MR targeting 9.3.x. In the future, please remember to close non-canonical merge requests, or at least indicate in the issue summary which is canonical and which should be closed. Thanks!

@xjm FYI, only maintainers of the project (in this case core contributors) or the original owner of the merge request can close it. I just checked this on the MR from @smustgrave and I don't see the "close merge request" button anywhere, tested without and with push access. Not sure if adding that to the IS helps as people usually tend to look at the comment or MR which is shown.

So not everyone who is triaging issues can do that 😉 valid point for hiding the patch files! Will try to remember that.

@BramDriesen Adding it to the IS does help as committers can close the incorrect ones, but only if they know which to close.

I do think the previous point of feedback about using the correct translation of empty is valid. Additionally, there are some minor coding standards issues. Thanks!

Saving issue credits.

I addressed the issues raised. I don't have a lot of experience with working on drupal.org issues via merge requests, so please do let me know if made a mistake.

I do think the previous point of feedback about using the correct translation of empty is valid.

I don't think this is the case: the callback is invoked when a user clicks on the "star"-icon to add a shortcut, so it only seems to make sense to add the shortcut in the active interface language.

I changed the translation source to "(Empty)" (which is already present in the core translation set.

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.

Mr.baileys theres 1000s of file changes now could you fix or revert your stuff back to before please

Tried to cherry pick the feedback change but had to force push to fix the MR.

Added one more comment and re-triggered the tests as they were failing.

Good catch!

I think you forgot to push your change 😇 I only see the rebase in your latest few commits

My mistake!

I think we can RTBC this now. Nightwatch is failing so re-triggered that as well.

Regarding:

I have fixed the sentence, but unsure about the @see: there is no module file for shortcut_test.

Eh? The code says:
Provides markup for the shortcut_test module.

If that doesn't exist, then what's the docblock referring to?

Added some screenshots

Did a translation and I'm seeing that

But will leave manual testing if needed

post_update should probably be a batch.

Added batch and fixed test

So in the related block content ticket that got merged they just returned a default vs saving one. Wonder if that’s what we should do here?

Hi folks, thanks for working on this issue.

I have a genuine question - what is the point of a shortcut with an empty title? How useful is a series of shortcuts all labelled `(Empty)`.

Instead of trying to resolve this - should we instead be preventing the shortcut functionality if there is no title. E.g. shortcut_preprocess_page_title if $name is empty, don't add the link - prevent people from adding empty shortcuts.

We can keep the update hook to fix broken items but I'm not convinced we want to let people create new empty links. Even then, should the update hook just delete the broken links instead of making them say '(Empty)'

Thoughts?

I’m fine with either approach just need to be agreed on. We’ve redone the solution multiple times it feels.

On D10.2.5, php 8.1 the patch #5 🐛 Shortcut links without a title cause deprecation notices. Needs work applies clean and fixes the "Deprecated function: strnatcasecmp(): Passing null to parameter #2" error.

I tried MR !5005 and it did not work. I was able to use #21 successfully.

I am on 10.2.6 using php 8.2.18

Thanks

I can confirm that MR !5005 doesn't work as well as #24.
But #21 works well.

I tested on:

Drupal 10.3.0
PHP 8.1.11

+1, I confirm that patch #21 works correctly with Drupal 10.2.3 and PHP 8.1.29

Based on #18, #22 and and #55, this is not yet RTBC. This is also tagged for manual testing. If that has happened the Issue Summary should be updated with a link to the comment where that was reported.

This needs discussion on the solution here. I certainly agree with #35 that this should be preventing the creation of shortcut when there is no title. I did see that there was concern about the usability of #18.2 so maybe a discussion is needed with usability, #ux, to get direction.