Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
After upgrading to Drupal 8.3.1 I've noticed drupal messages displayed by AJAX calls completely lose styling - the messages.css file is not loaded dynamically, tested on Seven and Bartik themes with the same effect.
Reproduce:
- add drupal_set_message('some message') in any AJAX form callback
- trigger AJAX on a page where no messages were displayed previously (messages.css wasn't already loaded)
May be related to:
https://www.drupal.org/node/1988968
Comment | File | Size | Author |
---|---|---|---|
#18 | 2875947-18.patch | 8.31 KB | alexpott |
#18 | 16-18-interdiff.txt | 1.28 KB | alexpott |
#16 | 11-16-interdiff.txt | 1.43 KB | alexpott |
#16 | 2875947-16.patch | 7.84 KB | alexpott |
#11 | 2875947-8.4.x-11.patch | 7.35 KB | alexpott |
Comments
Comment #2
Graber CreditAttribution: Graber as a volunteer commentedComment #3
cilefen CreditAttribution: cilefen commentedA `git bisect` is probably needed if we are not sure.
Comment #4
droplet CreditAttribution: droplet commenteddrupal_set_message set a flash message and displayed after a page reload, right? the CSS doesn't load from AJAX.
No errors on my testing (8.4.x)
EDIT:
and Bartik should always load the messages.css. @see bartik.libraries.yml
Comment #5
droplet CreditAttribution: droplet commentedFound the problem:
#2853509: Don't render status messages if there are no messages but also include their assets if there might be
Comment #6
alexpottHere's a fix. Basically prior to #2853509: Don't render status messages if there are no messages but also include their assets if there might be all classy themes were guaranteed to have classy's messages.css loaded - we can easily do just that.
Comment #7
alexpottIn fact here's a better fix that preserves the inheritance of libraries so overrides etc work as expected.
Comment #8
cilefen CreditAttribution: cilefen commentedComment #10
alexpott@xjm pointed out that we'd need a change clear to pick up the libraries change. Patch attached also fixes the tests. Big Pipe's life becomes a bit simpler because it no longer needs to add the messages library for status messages.
Comment #11
alexpottHere's specific tests to ensure that classy's messages.css is loaded for both classy and themes that extend classy like Bartik and Seven.
Comment #13
alayham CreditAttribution: alayham at EGHNA commentedthis might be completely related, or completely unrelated, depending on the approach:
#2876314: Allow a view to load style libraries
Comment #14
Graber CreditAttribution: Graber as a volunteer commentedThanks for the fast action!
The #10 fixed the problem, to be precise the single line added in classy.info.yml:
libraries:
- classy/base
+ - classy/messages
- core/normalize
I'll update the issue status to "fixed" when this will be released.
Comment #15
catchLet's add an empty post update rather than an empty hook_update_N() to minimize numbering conflicts and similar with other patches.
Also: shouldn't CSS added via AJAX callbacks be added to the page? We have a whole set of functionality around that in the AJAX system (ajax page state etc.) so I'm surprised it's not just working to be honest.
Comment #16
alexpottNice idea @catch - now one patch for 8.3.x and 8.4.x. Also @Graber emailed me and mentioned that the attach in classy's status-messages.html.twig is no longer necessary. Which is true.
@catch I don;t think the Ajax page state thing is working because the JS libraries for tabledrag don't depend on the message library - this is because the message library is only a thing in Classy and not core.
Comment #18
alexpottOne less new line in D8 output...
Comment #19
catchCan't classy add a hook_libraries_info_alter() to add itself as a dependency?
Comment #20
alexpott@catch but we can't know all the libraries in contrib and custom that would need this. It was added to every page.
Comment #21
nod_Comment #22
xjmRather than having a regression, a critical, a major, a BC break, and disruption to ongoing work, I've reverted the original issue and so closing this as duplicate. If someone can recapture the work here over there I'd appreciate it. Thanks!
Comment #23
Graber CreditAttribution: Graber as a volunteer commented@xjm if this is a duplicate, please provide the link to the original issue.
Comment #24
alexpott@Graber #2853509: Don't render status messages if there are no messages but also include their assets if there might be
Comment #25
Graber CreditAttribution: Graber as a volunteer commentedGreat, thanks!