Undefined $menu_item variable in layout_ajax_form_save_title_dialog()

[herbdool]

Description of the bug

Undefined $menu_item variable in layout_ajax_form_save_title_dialog().

I think it just needs $menu_item = $form_state['menu_item']; at the top of the function.

Looking at https://github.com/backdrop/backdrop/pull/1284/files it seems like menu_item would just be NULL anyway.

[indigoxela]

@herbdool that's a sparse report, how about adding steps to reproduce? :wink:

[herbdool]

@indigoxela I had noticed that it is undefined in my IDE, I haven't tried to get an error message in the browser.

[indigoxela]

I had noticed that it is undefined in my IDE

I tried to figure out, when this code ever runs, but wasn't able to. It's possibly dead code (or maybe not). Impossible to test. There are some oddities, anyway, with layouts and the page title form. For example, this is supposed to be a dialog, but it's unclear to me, how the form on admin/structure/layouts/manage/LAYOUT/edit-title/editor/title would ever show up in a dialog.

Giving up here with a shrug.

[argiepiano]

I wasn't able to make that portion of code run despite several different types of tries. An I agree there are several other oddities. The code in the function makes it look like ajax is involved, but in fact the menu definition for the path doesn't seem to invoke ajax rendering. In general, most dialogs I'm seeing within Layout UI are triggered through a link, but there are no links (other than the one in the admin menu) to access this form. Odd indeed.

[herbdool]

I've stirred up a hornet nest it seems. Maybe we should rip it out and see if anything breaks.

[indigoxela]

I've stirred up a hornet nest it seems.

Yes, I think you did. :laughing:

So... this came in with #952. The screenshots in that issue show what it was supposed to look like.

[indigoxela]

OK, the "Title" setting link opening a dialog shows up, as soon as neither the "Page title" nor the "Page title combo" blocks are present.

However, I still couldn't get the code to run, which tries to get the layout_locked_message() based on "menu_item".

BTW, the form lets me do all sorts of nasty mis-configurations, which ends up in loads of PHP warnings. Unrelated here.

The message built from the first clause is "This form has unsaved changes. Click "Save layout" to make changes permanent or "Cancel" to discard changes." and that message displays correctly.

Still no luck with the second where $menu_item is involved, as the first one's always true (?). The type is always layout. The layout_locked_message() function does consider the type "menu_item", but when would this ever strike on that page?

Maybe we should really rip out that "elseif"?

[argiepiano]

To add to the head scratching, this may have exposed another bug. The purpose of layout_locked_message() within layout_locked_message() is to check if the layout is being edited by another user. I'm able to trigger the message that indicates this by having two users simultaneously edit the layout, but only when I first open the layout edit UI in the second user:

But the layout CAN still be edited by the second user, and there are no messages indicating that it can't. I can edit the Page title (or any other block) normally and even save the layout.

[indigoxela]

The purpose ... is to check if the layout is being edited by another user.

Many thanks for digging that out @argiepiano Weird. I wasn't aware of that "locking feature" in our layout system, and it doesn't seem to work properly, anyway.

So we have a "nice" little bouquet of bugs.

• An undefined variable in use in a function without any doc block
• Weird UX with ajax-ish forms accessible without dialog - via admin bar
• That same form in dialogs via links under special circumstances
• A puzzling elseif clause that probably never runs (or does it?)
• Misleading use of a function supposed to check locking, but displays other messages
• But that locking doesn't even seem to be implemented (or is it?)

Great, we can now pick whatever we want to fix. :zany_face: Any preferences?

[indigoxela]

Oh, yet another little thing: that standalone title form allows setting a custom title also for layouts with placeholders in their path (like node/%).