Problem/Motivation
Discovered while investigating this bug in Claro #3067085: Views top dropbutton breaks when editing a view. The cause was narrowed down to the use of form alter for the use of hook_form_view_edit_form_alter()
. On the initial load, the UI is rendered with the alterations, but since the ajax re-generated markup isn't generated using form API, the alter hook isn't triggered. This is documented in Drupal\views_ui\ViewEditForm
and there's already hook_views_ui_display_tab_alter
to work around this problem. The hook isn't being triggered in themes so it cannot be used by Claro.
Proposed resolution
Trigger hook_views_ui_display_tab_alter
in themes as well so that Claro and other themes styling Views UI can use it. Since this is only for the tab contents, we have to also add a new alter hook hook_views_ui_display_top_alter
for altering the top region.
Remaining tasks
Get confirmation this is not a Claro issue- Review
- Commit.
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff.txt | 3.33 KB | lauriii |
#29 | 3067386-29.patch | 7.11 KB | lauriii |
#25 | 3067386-views-25-interdiff.txt | 3.55 KB | tim.plunkett |
#25 | 3067386-views-25.patch | 6.58 KB | tim.plunkett |
#23 | interdiff.txt | 1.3 KB | effulgentsia |
Comments
Comment #2
bnjmnmThe test largely recreates the situation that surfaced the issue in Claro. It uses form_alter within a
.theme
module that instructs menu_local_task elements in the Views UI to use a custom template. This custom template should always be the one used, but when unpatched the default template is used on ajax rebuilds.Comment #4
ryankavalsky CreditAttribution: ryankavalsky commentedPatch #2 works for me! Thank you!
Comment #5
Wim Leers😲 This is a pretty big change.
So time to dig into this! 🤓 I did manually reproduce the need for this by adding
to
seven.theme
.The original page load contains this HTML:
And after changing a filter in the view, I see this in the AJAX response:
So evidently the current render logic in Views UI is losing form alterations. Which is also the analysis in the issue summary.
🤔 All this complexity can go away — we should make this just a
<li>
plus that<span>
with text that we assert. The point is not the complexity of the template, but the fact that one or the other template is loaded. So let's keep the test as simple as possible to not distract from that :)🐛 This Claro-specific stuff should definitely not end up in core.
🐛 This needs cleaning up :)
🤔 AFAICT this is adding a theme suggestion, not merely ensuring it persists across AJAX rebuilds.
Just the act of adding this should already result in it persisting across AJAX rebuilds, because otherwise, what's the point?
IOW: this code looks good, but I think the comment needs to change.
Comment #6
lauriii+1 to the proposed solution in #5. It's an easy fix and will unblock Claro. Let's open a follow-up to solve the actual issue that form alter changes doesn't persist after ajax makes changes to the page.
Comment #7
huzookaI'm checking this.
Comment #8
huzookaAfter comparing the HTML output of #2 and #5, I realized that #5 changes the elements after a rebuild, and the attributes like 'data-drupal-selector', 'formnovalidate' and even the CSS ids of the
input[type="submit"]
elements aren't added to the output anymore.I feel this a bit dangerous. And since, for Claro, we have to alter the wrapper element of these
menu_local_task
s as well, and find a solution for the fake dropbuttons as well, #5 isn't enough there.I think that we should fix the actual issue.
Views display top HTML tree, default output and the output after a rebuild with patch #2:
Views display top HTML tree after a rebuild with patch #5:
Comment #9
tim.plunkettTo be safer, this should use ClassResolver to instantiate this. That way if ViewsUIController's constructor changes, we don't need to care.
If we're going to stop using these methods, we should deprecate/inline them.
Comment #10
Wim Leers😵
How is that possible?!
Comment #11
andypostI bet because states are bound to exact html-ids
Comment #12
lauriiiThis approach introduces a circular dependency between
Drupal\views_ui\ViewEditForm
andDrupal\views_ui\Controller\ViewsUIController
. We might want to avoid that if possible.Comment #13
Wim Leers#11: but that still doesn't explain why the patch in #5 would break this?
Comment #14
lauriii#13: The root cause for these problems is that the Views UI re-render completely by-passes form system. For example the
data-drupal-selector
attribute is added byDrupal\Core\Form\FormBuilder::doBuildForm
. Some of the side affacts has been documented in the code as well:I don't think this behavior is changed by #5. This is already like this in core.
Views UI provides some custom alter hooks which we might be able to use except that they are only triggered in modules. We should research if triggering
hook_views_ui_display_tab_alter
in themes would unblock Claro.Comment #15
tim.plunkettDiscussed at length with @lauriii. #5 at first seems like a stop-gap, but is insufficient. I will write a test that proves that we need something closer to #2. I will also address the rest of feedback from #5.
Also, even after fixing this bug, the change from #5 would be a nice improvement, but that should be left to #3051605: Create new template for rendering Views UI tabs..
Finally, #14 is 100% correct in that #5 does not cause any new bugs, the fact that the markup is missing the additional attributes is a direct effect of the bug itself.
Comment #16
xjmPromoting to major since this is blocking the inclusion of Claro in core.
Comment #17
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't know how close a proper solution to this issue is. If it's close, then ignore this comment.
If we want to explore a hack in the meantime...
Here's a Views UI patch that does that, along with the corresponding Claro patch. With very casual testing, it looks to me to solve #3067085: Views top dropbutton breaks when editing a view, but I did not perform a detailed inspection of the HTML source as #8 did.
Note that there are by design very few alter hooks that themes are allowed to participate in. I'm not at all keen on the idea of adding new ones that aren't well thought out, which is why this patch names them in a way to indicate their internal/temporary nature.
Comment #18
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAlternatively, we could check if Claro is the active theme or a base theme of the active theme, and if it is, call the Claro functions directly. That would be a way to more strongly prevent other themes from using these hooks, so that we don't have to preserve BC for them.
Comment #20
tim.plunkettCross-posted, so my patch numbers are wrong.
As #5.1 points out, this issue is really about changes from form_alter being lost. It's just coincidental that the way this bug was exposed was because of a form_alter making #theme changes.
#12 points out the circular dependency introduced by this approach.
And in #14, the question of
hook_views_ui_display_tab_alter()
is brought up. Views UI acknowledges that this code will not run in a form context, and so provides a hook. This is not documented anywhere but the hook invocation.Views UI's usage of AJAX and the Form API is very convoluted, and in some places even predates the APIs themselves. Untangling that is a much larger issue, and this fix maintains internal consistency.
Comment #22
tim.plunkettOpened #3087455: \Drupal\views_ui\ViewEditForm::rebuildCurrentTab() breaks hook_form_view_edit_form_alter() implementations for the larger issue.
#17 was right, there are two hooks needed.
This expands the docs for both hooks, and expands the test.
Comment #23
effulgentsia CreditAttribution: effulgentsia at Acquia commentedA bit more docs.
Comment #24
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCan we add this to
views_ui.api.php
instead?Comment #25
tim.plunkettWe sure can. Opened #3087465: Move hook_views_ui_display_top_links_alter() to views_ui.api.php to move the existing Views UI hook that is in the wrong place.
Comment #27
effulgentsia CreditAttribution: effulgentsia at Acquia commented#25 looks great to me, and as a Theme API maintainer, I even accept the two alter hooks being routed to themes, despite what I wrote in #17 about not being keen on that. I think the docs in #23 explain the rationale for doing so.
Comment #28
xjmOverall this looks good; just a few nitpicks...
The inline comments document that we're implementing this as a workaround, which would make me expect a link to the followup to fix it (#3087455: \Drupal\views_ui\ViewEditForm::rebuildCurrentTab() breaks hook_form_view_edit_form_alter() implementations I think). It's documented on the hook docs, but not here where the hooks are invoked. Probably we want the
@todo
in both places.Oh yay, another place that's gonna need to be fixed in #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed. :P
I think perhaps it implements views UI alter hooks in a theme to provide test coverage for them. :)
We usually like to be more specific than
array
; since it's a render array, is it anarray[]
at least?Comment #29
lauriiiThis should address feedback from #28. 🤞
Comment #30
tim.plunkettWhile I worked on the most-recentely-RTBC patch, I have reviewed the changes since then and can sign off that they are correct and address #28. Thanks @lauriii!
Comment #31
larowlannow we have support for setting the default theme via a test-class property, should we split this into its own class and make use of that property?
Comment #32
larowlanAlso, we need an issue summary update here, as the proposed resolution doesn't resemble the patch.
Tentatively tagging for needs change record too, on the basis of the new hook and making two new hooks available to themes, although given the invocations both have a todo, I'm not sure if we're advertising them? Their presence in .api.php makes me think so, so I added the tag.
Comment #33
larowlanHappy if the answer to #31 is » followup issue
Comment #34
lauriiiUpdated issue summary and added change record: https://www.drupal.org/node/3088233.
Opened follow-up for #31: #3088234: Use $defaultTheme for using views_test_classy_subtheme.
Comment #35
larowlanComment #36
larowlanComment #39
larowlanCommitted 5ed0049 and pushed to 9.0.x. Thanks!
backported to 8.9.x
published the change record
moving to 8.8.x for decision about backport
Comment #41
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.8.x and updated the CR to that version.