Problem/Motivation

The template_preprocess_image_widget function generates some markup which isn't overridable by themers without overriding array by duplicating the logic.

Proposed resolution

Move the markup generation from preprocess function to the template.

Remaining tasks

User interface changes

N/a

API changes

New file_size theme variable in file-link theme

Data model changes

N/a

Release notes snippet

@todo

CommentFileSizeAuthor
#13 interdiff.txt948 byteslauriii
#13 3033279-13.patch3.8 KBlauriii
#10 interdiff.txt2.35 KBlauriii
#10 3033279-10.patch3.63 KBlauriii
#8 interdiff.txt2.2 KBlauriii
#8 3033279-8.patch2.5 KBlauriii
#6 3033279-6-template_preprocess_image_widget-markup-to-template.patch2.05 KBmandclu
#5 3033279-5-template_preprocess_image_widget-markup-to-template.patch0 bytesmandclu

Comments

lauriii created an issue. See original summary.

driverok’s picture

driverok commented
Issue tags: +epam-contrib-2019.03
driverok’s picture

driverok commented
Issue tags: -epam-contrib-2019.03

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mandclu’s picture

mandclu commented
Status: Active » Needs review
StatusFileSize
new 3033279-5-template_preprocess_image_widget-markup-to-template.patch0 bytes

Attaching a patch that moves the markup to templates within the file module (in both the core module and classy theme), calculated within the file module.

mandclu’s picture

mandclu commented
StatusFileSize
new 3033279-6-template_preprocess_image_widget-markup-to-template.patch2.05 KB

Proper patch attached this time.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Admin UI Modernisation initiative
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new 3033279-8.patch2.5 KB
new interdiff.txt2.2 KB

I removed changes made to Classy and added code for adding the #suffix back in Stable so that theme extending Stable will remain unchanged.

eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/file/file.module
@@ -1492,6 +1492,11 @@ function template_preprocess_file_link(&$variables) {
+  // Set the filesize

    Coding standards nit - we should end // comment lines with a . 

  2. +++ b/core/modules/file/file.module
@@ -1492,6 +1492,11 @@ function template_preprocess_file_link(&$variables) {
+  $variables['filesize'] = format_size($file_entity->getSize());

    Where does this $file_entity come from? Should this be $file? 

  3. +++ b/core/modules/file/templates/file-link.html.twig
@@ -13,3 +13,7 @@
+{% if filesize %}

    We should document the filesize variable in the docblock

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3033279-10.patch3.63 KB
new interdiff.txt2.35 KB

It seems like #9.2 is caused by #3055474: template_preprocess_file_link will not work with a stdClass object, though it tries to.

Fixed all and renamed filesize to file_size.

eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented
Status: Needs review » Reviewed & tested by the community

Reviewed the patch in #10 and confirm all concerns in #9 addressed. Have tested display of the file link in a custom theme with no base theme and the display is identical with and without this change so no regression in evidence. Also agree with the renaming of the filesize variable to file_size.

Therefore moving to RTBC.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Added credit for @Eli-T for a thorough review.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new 3033279-13.patch3.8 KB
new interdiff.txt948 bytes

@alexpott recommended adding documentation to the Stable and Classy to point to the newly added Stable preprocess function. Keeping RTBC since this is a small documentation change.

I also tested this with Bootstrap since they use the same variable name to make sure that it works as expected. Everything seemed to work as usual.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Issue tags: +Needs change record, +8.8.0 release notes

We need a change record for the additional of the theme variable and probably we could do with a release notes section to the issue summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -Needs change record, -8.8.0 release notes

I created a change record for this.

I don't think we have to include this in the release notes given that this isn't disruptive change. This is also not a major or critical issue that we should highlight in the release notes.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Creating @Eli-T as per #12

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed cfd9934 and pushed to 8.8.x. Thanks!

  • alexpott committed cfd9934 on 8.8.x 
    Issue #3033279 by lauriii, mandclu, Eli-T: Move markup from...

Status: Fixed » Closed (fixed)

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

marassa’s picture

marassa commented

I am confused: the committed patch actually ADDED some arbitrary markup generation into stable.theme that had never even been there, contrary to what was supposed to be done in the issue.

I had a preprocess_image_widget hook in my module that perfectly worked until I upgraded to 8.8. Took me hours to figure out why the output of my hook is ignored or overwritten, now it turns out that whatever I put in #suffix in my module will be overwritten by the theme...

Moving my hook to my theme won't work because I mostly need this functionality in admin/edit mode where I use Seven theme...