Remove drupal_add_css() from book.module - use #attached instead.

Meta issue: #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff

CommentFileSizeAuthor
#11 2012526-11-remove-drupal_add_css.patch1016 bytesmcjim
#9 2012526-8-remove-drupal_add_css.patch1012 bytesmcjim
#3 2012526-3-remove-drupal_add_css.patch1008 bytesmcjim
#1 2012526-remove-drupal_add_css.patch2.07 KBmcjim

Comments

mcjim’s picture

mcjim commented
Status: Active » Needs review
StatusFileSize
new 2012526-remove-drupal_add_css.patch2.07 KB

Removed drupal_add_css from template_preprocess_book_navigation().

CSS is #attached in three different places:

In book_node_view() for the navigation below the node.

In BookNavigationBlock twice, for the block shown on all pages and the block only shown on book pages.

mcjim’s picture

mcjim commented
Status: Needs review » Needs work

Actually, scratch that. The CSS is not needed by the blocks at all. Re-rolling…

mcjim’s picture

mcjim commented
Status: Needs work » Needs review
StatusFileSize
new 2012526-3-remove-drupal_add_css.patch1008 bytes

Re-rolled. CSS is #attached only in book_node_view().

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: +Needs manual testing

Code looks good. This needs manual testing, however, to verify that the CSS is still being applied in all cases.

ekl1773’s picture

ekl1773
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Steps taken to test:

  1. Created several articles and book pages
  2. Added articles/pages to new book with different weights and parentage
  3. Viewed book, checked if CSS loaded by:
    • Inspecting individual elements in the Elements tab and checking their CSS inheritance
    • using jQuery to find known selectors from the theme
  4. Everything checks out, the book.theme.css loads successfully.

However, when I go to edit a book page (but NOT an article) or view the outline, the following error appears above the edit box:

Notice: Trying to get property of non-object in Drupal\menu_link\MenuLinkStorageController->findChildrenRelativeDepth() (line 567 of core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php).
Notice: Trying to get property of non-object in Drupal\menu_link\MenuLinkStorageController->findChildrenRelativeDepth() (line 573 of core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php).
Notice: Trying to get property of non-object in Drupal\menu_link\MenuLinkStorageController->findChildrenRelativeDepth() (line 580 of core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php).

mcjim’s picture

mcjim commented

Thank for the thorough review!

I don't think those notices have anything to do with this patch (might even be related to #2012920: Add child page link in book navigation does not insert parent into book outline), but will check them out.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Reviewed & tested by the community » Needs review

So can someone reproduce the problem without the patch?

mcjim’s picture

mcjim commented

Re-rolled patch to keep up with HEAD.

re: #5 and #7

I think it's something to do with caching.

Can you try this:
1) Pull latest changes.
2) Apply patch
3) Do a clean install (will need to remove sites/default/files/php and sites/default/files/config_*, first).
4) Test creating a book.

I've just done this and have no errors.

mcjim’s picture

mcjim commented
StatusFileSize
new 2012526-8-remove-drupal_add_css.patch1012 bytes

Did that forgetting to upload the patch, thing…

ekl1773’s picture

ekl1773
she/her
Primary language English
commented

Yes, tested on clean copy of 8.x, aforementioned error exists irrespective of this patch.

Pulled, applied, wiped sites/default/files etc, tested as above.

Some CSS appears to have loaded, but book.theme.css throws a 404 error when selected in the resources tab and the elements report that they're drawing CSS from themes/bartik, which does not include the subelement selectors like "next" and "up."

Either needs re-rolling or it's me?

mcjim’s picture

mcjim commented
StatusFileSize
new 2012526-11-remove-drupal_add_css.patch1016 bytes

Ah, the 404 is explained by http://drupalcode.org/project/drupal.git/commit/2432c02 which moved the CSS.
Rerolled!

ekl1773’s picture

ekl1773
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Ok, that appears to work, book.theme.css loads, elements are displayed accordingly, tested with several content types. Ditto above, but additionally: success!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.