Problem/Motivation
Book module renders export pages with custom page template that should contain "print.css" but this broken because asset hardcoded relatively to current page.
Also there's no way to alter assets attached to the page because response is returned from controller without processing assets (to prevent unneeded styling)
Steps to reproduce:
1) enable book module and create new book node
2) visit "Printer-friendly version" like /book/export/html/1
3) make sure print.css
loaded
currently it's broken because path should be core/misc/print.css
Proposed resolution
Make new library core/drupal.print
with print.css
Allow theme libraries to override/extend drupal.print
library
Add tests and commit
Remaining tasks
find a way to render attached libraries in custom responses that should skip page template from active theme
User interface changes
libraries attached in book export pages should be rendered
API changes
no
Original report by [@pianomansam]
The book module should really have the capability to pull in the print stylesheets from the active theme. Right now it is hardcoded in book-export-html.tpl.php to only pull in misc/print.css and getting it to do more requires copying the template file into the theme directory and manually pulling in a print stylesheet. This is very hackish and uncharacteristic of the Drupal Way.
Comment | File | Size | Author |
---|---|---|---|
#23 | 1338858_23.patch | 3.97 KB | Mile23 |
#9 | 1338858-print-css-9.patch | 2.73 KB | andypost |
Comments
Comment #1
watbe CreditAttribution: watbe commentedJust encountered this issue. Should be a simple fix.
Comment #2
watbe CreditAttribution: watbe commentedI can't find the function that would load a theme's defined stylesheet. Is that even possible? Need to load the stylesheets defined in a theme's .info as [print].
Updating tags.
Comment #2.0
watbe CreditAttribution: watbe commentedcorrecting typo
Comment #3
clemens.tolboomCurrent implementation is still the same but now as a twig template.
Comment #4
andypostThis is a bug, discovered working on #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.
So templates hardcodes wrong path
Comment #5
andypostComment #6
jaimeguzman CreditAttribution: jaimeguzman at Skilld commentedWe found this bug with andypost try it to fix this: https://www.drupal.org/node/2477223#comment-9922524
The book export show a missed path for css, also I think that not have to showed.
Comment #7
clemens.tolboom@jaimeguzman the original poster want drupal to include a print.css from the current theme.
Please create a https://www.drupal.org/patch file :)
Comment #8
andypostUpdated IS with current state.
The actual problem that there's no library that themes can override/extend with custom css also because assets are not processed.
Book module can't execute
template_preprocess_page()
to prevent other assets attached.WIP patch, still broken.
Comment #9
andypostSo only
media: print
assets should be renderedComment #10
andypostComment #13
Wim LeersThis cannot possibly work because the HTML main content renderer is not involved, which means a lot is bypassed.
I'll need to debug this to figure out how this actually should be implemented.
Comment #14
dawehnerSo what are we trying to achieve here?
Comment #15
andypostAs I get current render I need to run a main content rendered on that custom template.
Otoh there some
SimplePageVariant
to exclude blocks.Currently book export render mostly broken because it was forgotten to to update with current render/cache changes, so maybe better to #1843718: Remove "export" functionality from Book module, apply to all node types at the current stage
Comment #16
Crell CreditAttribution: Crell at Palantir.net commentedThis sounds like another use case for a custom rendering context. Have a look at Aggregator module, which I think is the only other example in core. (Rendering context is not currently an explicit thing, although it probably should be.) You have to build a render array and call the renderer yourself. Do NOT call any render/markup functions directly, as those are all vestigial and will hopefully die soon.
Comment #17
andypostThe only example I found in core is
\Drupal\views\Plugin\views\display\Feed::buildResponse
andtemplate_preprocess_views_view_rss
So looks here we need to do the same
CacheableResponse
that uses custom render and custom asset attach...@Crell there's no "rendering context" in core yet
Comment #18
andypostAnother way is to use
bare_html_page_renderer
service!\Drupal\system\Controller\DbUpdateController::handle()
uses that oneComment #19
Crell CreditAttribution: Crell at Palantir.net commentedRelated, which will provide a clearer API for render context: #2450993: Rendered Cache Metadata created during the main controller request gets lost
Comment #20
Crell CreditAttribution: Crell at Palantir.net commentedThe API rendering context issue is in, so this issue should be doable now.
Comment #21
joelpittetComment #22
Mile23If by 'another' you mean "...in addition to the
renderer
service..." then yes. These should be handled through the available services, and no other way. If there's no way to do that, then the solution is to fix the services so they can provide the use-case.Also, related: #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. has to comment out a line from BookTest to pass the testbot, so we're in a bit of a holding pattern on this one.
Comment #23
Mile23This patch sort of halfway works. :-)
We have BookController use the bare_html_page_renderer service to generate a Response object and return that. I'm under the impression that a Response object should bypass normal theming when returned from a controller.
However this led to the content being themed twice, so I deleted the twig file from bartik. I figure this is OK since the premise here is that the book export should have its own theme anyway.
The big problem, however, is that something is outputting the HTTP headers as part of the content. I assume that's the kernel getting mixed up in some way, and not a problem with book.
And of course the css itself could probably use some love, but I can't get far enough to address that.
Also I just started working without all the proper rigamarole so I don't have an interdiff. Sorry.
Comment #27
mgiffordComment #36
saphemmy CreditAttribution: saphemmy at CI&T commentedComment #39
quietone CreditAttribution: quietone at PreviousNext commentedThis extension is being deprecated, see #3376070: [Meta] Tasks to deprecate Book module. It will be removed from core and moved to a contrib project, #3376101: [11.x] [Meta] Tasks to remove Book.
This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.
This issue may be re-opened if it can be considered critical, If unsure, re-open the issue and ask in a comment.
Comment #40
quietone CreditAttribution: quietone at PreviousNext commentedUn-assigning per Assigning ownership of a Drupal core issue.