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

Files: 
CommentFileSizeAuthor
#11 2012526-11-remove-drupal_add_css.patch1016 bytesmcjim
PASSED: [[SimpleTest]]: [MySQL] 55,404 pass(es). View
#9 2012526-8-remove-drupal_add_css.patch1012 bytesmcjim
PASSED: [[SimpleTest]]: [MySQL] 55,278 pass(es). View
#3 2012526-3-remove-drupal_add_css.patch1008 bytesmcjim
PASSED: [[SimpleTest]]: [MySQL] 56,062 pass(es). View
#1 2012526-remove-drupal_add_css.patch2.07 KBmcjim
PASSED: [[SimpleTest]]: [MySQL] 55,352 pass(es). View

Comments

mcjim’s picture

Status: Active » Needs review
FileSize
2.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,352 pass(es). View

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

Status: Needs review » Needs work

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

mcjim’s picture

Status: Needs work » Needs review
FileSize
1008 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,062 pass(es). View

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

tstoeckler’s picture

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

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

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

Status: Reviewed & tested by the community » Needs review

So can someone reproduce the problem without the patch?

mcjim’s picture

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

FileSize
1012 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,278 pass(es). View

Did that forgetting to upload the patch, thing…

ekl1773’s picture

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

FileSize
1016 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,404 pass(es). View

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

ekl1773’s picture

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

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.