Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Issue #1898442 by Cottser, duellj, kgoel, tlattimore, cyborg_572, rwohleb: responsive_image.module - Convert theme_ functions to Twig.
Task
Use Twig instead of PHPTemplate
Remaining
- New patch needs to be created now that #2260061: Responsive image module does not support sizes/picture polyfill 2.2 is in.
Theme function name/template path | Conversion status |
---|---|
theme_responsive_image_formatter | todo |
API changes
TBD
To test this code:
- Enable responsive image module
- Add a new picture mapping (admin/config/media/responsive-image-mapping) with
"Bartik" as the breakpoint group and different image styles for each of the
three breakpoints (e.g. 1x mobile = Thumbnail) - Alter the image field on Article nodes to use picture formatting (admin/structure/types/manage/article/display)
- Select the responsive image mapping that you just created in the formatting
settings - Create a new article node with an image
- Resulting node should output image using picture templates (with
<picture> and <source> tags)
Comment | File | Size | Author |
---|---|---|---|
#187 | diff-185-187.txt | 1.67 KB | jayeshanandani |
#187 | 1898442.187.patch | 22.26 KB | jayeshanandani |
#185 | interdiff-181-185.txt | 4.87 KB | jayeshanandani |
#185 | 1898442.185.patch | 22.29 KB | jayeshanandani |
#182 | diff 171-181.txt | 8.38 KB | jayeshanandani |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
GeminiAgaloos CreditAttribution: GeminiAgaloos commentedComment #3
GeminiAgaloos CreditAttribution: GeminiAgaloos commentedAt SandCamp 2013 twig sprint: On my first Drupal sprint after Ezra Gildesgame's keynote speech earlier today encouraging us to contribute even (especially?) when we've never done a drupal sprint before :)
Comment #4
star-szrUnassigning since it's been a while. @007g3m1n1 - if you'd still like to work on this, please re-assign to yourself. Thanks!
Comment #5
duellj CreditAttribution: duellj commentedI'm going to give this conversion a go. Any objections to doing all work here, rather then in the three issues in the twig sandbox?
Comment #6
duellj CreditAttribution: duellj commentedHere's the first pass at converting all picture themes to twig. One thing to note is that it looks like there was some misnamed variables in picture_theme() that I had to change in order for tests to pass, since I'm creating render arrays in preprocess functions now.
Comment #8
duellj CreditAttribution: duellj commentedWhoops, tests should pass now.
Comment #8.0
duellj CreditAttribution: duellj commentedUpdated issue summary. Added related issues.
Comment #9
star-szrTagging.
Comment #9.0
star-szrAdd conversion summary table
Comment #10
star-szrThis is looking really good, mostly nitpicky things. I added the two variable changes to the issue summary under API changes.
Documentation
The preprocess functions docblocks all look good, but we should be removing @ingroup themeable from them, we are adding @ingroup themeable to the Twig templates instead. See #1913208: [policy] Standardize template preprocess function documentation. We can also update the @param to
@param array $variables
on a couple of the preprocess functions.This is not docs only - the documentation confused me a bit, the variable description almost makes it sound like the attributes would be at path.attributes, and the preprocess function looks like it puts the attributes there. So I think the comment is probably correct but it should be {{ path.attributes }}, and butted up against the href="".
We're also trying to avoid putting the word "array" in Twig templates, so maybe something like this?
It would be nice if this explained briefly what a srcset is, and what multipliers are :) Two "the" here on the first line as well.
Capitalize "Additional".
Code
Since preprocess functions modify the variables by reference, there shouldn't be a return.
Not sure about this hunk, in one case attributes would be an array, in the other it would be an Attribute object. Do we need the empty array here? If so, I think it should be
new Attribute()
instead ofarray()
.Comment #11
star-szrDidn't mean to remove the tag. @duellj said he could add manual testing steps to the summary.
Comment #11.0
star-szrAdd API changes
Comment #12
duellj CreditAttribution: duellj commentedUpdated patch with fixes from #10, thanks Cottser!
A couple notes:
The return was there to break out of the function so further variable preprocessing wouldn't happen. I wrapped it in an else{} instead.
Good call, I've changed it to create a new empty Attribute() if there's no attribtues for the path.
Also updated the issue with manual testing steps.
Comment #13
star-szrI tested this manually and found one small discrepancy in picture-source. In the after-conversion markup, the second
<source>
element didn't have width and height attributes.Before:
After:
The Attribute class keeps track of which attributes are printed so that we can do things like
<tag class="{{ attributes.class }}"{{attributes}}>
without having the classes print twice (http://drupal.org/node/1727592).Attached fixes this issue by setting a new variable in the Twig template to allow for repeated printing of the attributes. Also made all {{ attributes }} butt up against preceding elements as per Twig coding standards and tweaked the closing slashes on the
<source>
tags to be consistent.Comment #14
star-szrImproved the comment explaining why we set a new variable.
Comment #15
star-szrIgnore #14, interdiff here is from #13.
Comment #16
star-szrAssigning this one to @steveoliver to review, I think this is ready to go.
Comment #17
steveoliver CreditAttribution: steveoliver commentedThis var change seems reasonable to me--yes an API change, but more like a clean-up task as 'dimension' doesn't really make sense.
Let's keep to the ternary form when setting $variables['path']['attributes'], yeah?
Like this:
$output = array(); can be deleted.
"Prepares variables for /picture source/ templates."
1. The form of
{% set src_attributes %}{{ attributes }}{% endset %}
should be{% set src_attributes = attributes %}
. Also let's call that var 'attributes_copy' or something that doesn't blur in with all the other 'src' text throughout this template.1.b. Also, the comment above, "...as many times as we'd like" I think should just be "...a second time for the commented source tags."
2. Since {{ attributes }} and {{ attributes_copy }} (or whatever we call it) are each only being printed once, let's use them like this:
{{ attributes_copy }} for the commented-out
<source
tags, and {{ attributes }} for the non-commented-out<source
tags.3.
if src / elseif srcset
should use empty tests, so we should have{% if src is not empty %}
...{% if srcset is not empty %}
. This will keep things working if we ever implement strict variables Twig, where an undefined src/srcset would throw errors without theempty
(oris defined
) check.4. Actually, since all of the variables are actually attributes, why not put them into the attributes variable?. As is it, the only attributes are width and height dimensions, which with the current docblock, is actually not clear.
The docblock would need restructuring, and our template code would look more like this:
Edits:
1. attributes: HTML attributes for /the/ picture /tag/.
2. fallback: Fallback image /tag/ for non-JS browsers.
Comment #18
star-szrI'm on it, thanks for the thorough review @steveoliver!
Comment #19
star-szrThis should address all of #17, other than these exceptions:
I think "source tag templates" is valid, picture-source.html.twig outputs the
<source>
tags, picture.html.twig outputs the<picture>
tag. I could go either way on this though.{% if src is not empty %}
on IRC, the 'is not empty' is not needed in this case.Changes:
Now template_preprocess_picture_source() builds the Attribute object based on the passed variables and creates two variables to allow for printing both in the template. picture-source.html.twig is much simpler now that everything is in attributes.
Markup tested again and still matches up with HEAD.
Comment #20
steveoliver CreditAttribution: steveoliver commentedUpdate: will be reviewing this Thursday, incase anyone else wants to review/RTBC in the meantime.
In the meantime, a few things that
API change.
Is it worth doing this? If so, why? and we'll need a change notice.
API change/cleanup (typo fix). Change notice?
Comment #21
tlattimore CreditAttribution: tlattimore commentedre #20: I am with steveoliver. I don't think we need to change
path
touri
. Path is pretty commonly used across core for things url's passed. Re-rolled patch and removed this change (see interdiff.txt).I do however think that
dimensions
should expect an array as it's value and not NULL, as this will always be an array. So I am tagging for change notification to be done.Comment #23
star-szrWe'll need to change 'uri' back to 'path' throughout the patch.
Comment #24
tlattimore CreditAttribution: tlattimore commentedI believe what cottser is saying is that 'path' needs to be changed to 'url' across the whole patch and not just one place like I did in #21. Correct?
Comment #25
kgoel CreditAttribution: kgoel commentedGoing to work on this issue.
Comment #25.0
kgoel CreditAttribution: kgoel commentedAdds testing steps
Comment #26
kgoel CreditAttribution: kgoel commentedPatch attached.
Comment #27
kgoel CreditAttribution: kgoel commentedReplaced uri with path in picture.module. Templates look fine.
Comment #29
kgoel CreditAttribution: kgoel commentedAnother try
Comment #31
star-szrThanks @kgoel, you did good in the end :) I wasn't careful enough when I was looking at this with you last night, I will straighten this out later today. I wasn't clear enough in my task description in #23 and this rollback requires a bit of familiarity with this issue's history.
Comment #32
star-szrtheme_picture() before this patch was not even using the correct variables. picture_theme() declares:
But then theme_picture() looks like this:
So things should be good now. Interdiff is from #29.
I did another manual test and compared markup via visual diff and DaisyDiff and markup is still looking good.
Back to you, @steveoliver :)
Comment #33
star-szrAnd here are the missing docs based on the hook_theme() declaration. Grabbed 'title' and 'style_name' descriptions from theme_image_style().
Comment #34
steveoliver CreditAttribution: steveoliver commentedI got through maybe a 50% review of this. For now:
1. DocBlock change
1.a. Change "One of the following two variables will be available" to "One of the following two attributes will also be set."
1.b. attributes_copy: Change "A copy of the HTML attributes in the 'attributes' variable..." to just "A copy of the 'attributes' variable..."
2. All the conditions render the same tags. I think we literally need zero checks and only the two source tags.
3. It'd really make sense if the 'path' var were named 'link', as that's actually what it is and that's how we describe it. Could be non-Twig API follow-up, perhaps.
Comment #35
star-szrRelating to #3, those shouldn't be documented as path.path and path.attributes either, just 'path' and 'attributes'. Indent means 'add a dot'.
Great catches @steveoliver, tagging as a good novice task.
Comment #36
cyborg_572 CreditAttribution: cyborg_572 commentedComment #37
cyborg_572 CreditAttribution: cyborg_572 commentedForgot to update issue status.
Comment #38
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.
Comment #39
attiks CreditAttribution: attiks commentedFYI: keep an eye on #1883526: Decide on the picture polyfill to use, because the output will no longer use picture and source tags
Comment #40
webthingee CreditAttribution: webthingee commentedUnable to find the process described in Step 3 for testing.
Comment #41
jstoller#36: 1898442-36.patch queued for re-testing.
Comment #43
jrglasgow CreditAttribution: jrglasgow commentedI will attempt to get the patch working
Comment #44
jrglasgow CreditAttribution: jrglasgow commented#36: 1898442-36.patch queued for re-testing.
Comment #45
jrglasgow CreditAttribution: jrglasgow commentedComment #46
rwohlebRunning profile against 1898442-36.patch
Comment #47
rwohleb=== 8.x..8.x compared (519ff47950466..519ff57991ea2):
ct : 586,572|586,572|0|0.0%
wt : 5,528,379|5,523,830|-4,549|-0.1%
cpu : 5,438,279|5,428,694|-9,585|-0.2%
mu : 53,683,752|53,684,856|1,104|0.0%
pmu : 53,875,312|53,876,392|1,080|0.0%
Profiler output
=== 8.x..issue-1898442 compared (519ff47950466..519ff595715f0):
ct : 586,572|586,887|315|0.1%
wt : 5,528,379|5,556,077|27,698|0.5%
cpu : 5,438,279|5,460,500|22,221|0.4%
mu : 53,683,752|53,687,264|3,512|0.0%
pmu : 53,875,312|53,878,152|2,840|0.0%
Profiler output
Comment #48
benjy CreditAttribution: benjy commented@rwohleb, links to the profiler output seem to be your local machine.
Comment #49
rwohleb@benjy, I forgot to remove those links from the xhprof-kit output. I don't have an API key yet to get it online.
I realized there was an issue with my previous profiling, so here is an updated version.
=== 8.x..8.x compared (519fff4bc13bf..51a0008669b58):
ct : 610,847|610,847|0|0.0%
wt : 5,733,916|5,743,103|9,187|0.2%
cpu : 5,638,489|5,641,707|3,218|0.1%
mu : 58,147,712|58,147,712|0|0.0%
pmu : 58,361,520|58,361,520|0|0.0%
=== 8.x..issue-1898442 compared (519fff4bc13bf..51a000a29e44d):
ct : 610,847|611,132|285|0.0%
wt : 5,733,916|5,700,138|-33,778|-0.6%
cpu : 5,638,489|5,599,642|-38,847|-0.7%
mu : 58,147,712|58,151,928|4,216|0.0%
pmu : 58,361,520|58,366,384|4,864|0.0%
Comment #49.0
rwohlebRemove sandbox link
Comment #49.1
ezeedub CreditAttribution: ezeedub commentedUpdated issue summary.
Comment #50
joelpittetThis one needs a re-roll.
Comment #51
duellj CreditAttribution: duellj commentedHere's a reroll of the patch from #36
Comment #52
jenlamptonupdating tags
Comment #52.0
jenlamptonadd to related
Comment #53
azinoman CreditAttribution: azinoman commentedComment #54
adamcowboy CreditAttribution: adamcowboy commentedThe patch works! It resizes correctly.
Comment #55
adamcowboy CreditAttribution: adamcowboy commented(forgot to change)
Comment #56
star-szrWould like another round of manual testing please, since it's been a while.
We'll also need to remove these lines per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.
Comment #57
andypostRelated #2029649: Move entity_load('image_style', ...) in preprocess
Comment #58
azinoman CreditAttribution: azinoman commentedApplied Cottser's changes. And rerolled patch. Needs Review
Comment #59
azinoman CreditAttribution: azinoman commentedWhoops. Forgot to attach the file
Comment #60
joelpittetYou got some whitespace issues in there, you can just remove that entire line not just the @see
Comment #61
jenlamptonAlso looks like this patch includes changes for aggregator module. Make sure you stash your previous changes before starting on a new issue :)
Comment #62
jenlamptonOkay, here's a reroll of the patch in #52, with interdiff.
Also, manual testing results:
Before
After
Comment #63
joelpittetRTBC as per #56 being covered.
Comment #64
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #65
alexpottThe logic here is not quite equivalent... the original function is extremely messy but it seems to have the following qualities that the new one does not have. I do not know if this is important this is just from reviewing the code. These qualities were:
media
was not set assumesrc
is set and use it.media
is set andsrcset
is not not set assumesrc
is and use it.media
is set andsrc
is not not set assumesrcset
is and use it.The point being is that it was impossible to have both src and srcset as properties on the same tag... it now is...
I'm more than happy to convince that this is not a problem. The original logic is full of code smells to me. And seems to be undocumented as to why :)
Comment #66
attiks CreditAttribution: attiks commented#65 It's important, we can use either src or srcset, not both at the same time, but the code only sets one, so I think we are lacking some documentation, feel free to add the comments or create a follow up and I'll add them.
According to the working draft, both can be defined at the same time (and even on the picture element as well. The current polyfill will first check srcset first and assumes a src attribute in the other case.
The specs says: "There are three ways to specify an image resource, the src attribute, the srcset attribute, or source elements. The srcset attribute overrides both the src attribute and the elements; the src attribute overrides the elements."
Comment #67
jenlamptonAfter reading #67 it sounds like the new, cleaner code is fine as-is. changing back to RTBC.
Comment #68
effulgentsia CreditAttribution: effulgentsia commented#62 doesn't apply to HEAD.
Comment #69
Les LimRe-rolling #62.
Comment #70
Les LimReroll changes
image_style_url()
calls toentity_load('image_style')
.Comment #72
Les LimUpdated patch to use $variables['path'] instead of $variables['uri'].
Comment #73
Les LimUnassigning.
Comment #74
joelpittetBack to RTBC, thanks @Les Lim
Comment #75
alexpott@Les Lim - why did you change path to uri ... in the issue summary it states that this patch is changing path to uri.
Let's use uri where it contains a string that will be used to build the uri and path where it contains an array with options for url() or l()
Comment #76
Les LimHm, I'm about to go to bed, but if I recall,
$variables['uri']
isn't an index that gets passed to the preprocess function. That's where the 16 exceptions from the straight reroll at #70 come from.$variables['path']
is available, though. I can go back and verify this in the morning.Comment #77
pplantinga CreditAttribution: pplantinga commentedThis issue has been fixed in #2026319: Mis-named variables in picture_theme() so we can go back to using $variables['uri']
Comment #78
pplantinga CreditAttribution: pplantinga commented#70: core-1898442-70-picture_module_to_twig.patch queued for re-testing.
Comment #79
pplantinga CreditAttribution: pplantinga commentedReroll of #70
Comment #81
thedavidmeister CreditAttribution: thedavidmeister commentedThis will probably need a re-roll if #2009662: [REGRESSION] Replace theme() with drupal_render() in picture module lands.
Comment #82
alexpott#2009662: [REGRESSION] Replace theme() with drupal_render() in picture module has landed...
Comment #83
steinmb CreditAttribution: steinmb commentedWill be working on this
Comment #84
xjmWe only add the "Needs change notification" tag after commit since things can change. :)
Comment #85
pplantinga CreditAttribution: pplantinga commentedreroll of #79
Comment #87
pplantinga CreditAttribution: pplantinga commentedMissed one.
Comment #89
pplantinga CreditAttribution: pplantinga commentedone more.
Comment #90
steinmb CreditAttribution: steinmb commentedThanx for re-rolling. I started on it did not work 100% after I rebased it and then I got swamped :-/
Testing #89 on HEAD of 8.x
So looking at this it seems to do it's thing but I have problem getting access to the generated versions of the attached image, it keep giving me a 403.
Resizing my browser window and firebug throws:
And yes, loading the generated version. example http://d8.dev/sites/default/files/styles/thumbnail/public/field/image/gr... works just fine.
Comment #91
steinmb CreditAttribution: steinmb commentedBacktrace from drupal log:
Comment #92
pplantinga CreditAttribution: pplantinga commentedI guess this needs work then...
Comment #93
steinmb CreditAttribution: steinmb commentedSome debugging information dumped from template_preprocess_picture_formatter()
variables['image'] :
Looks right to me. With public://field/image/brown.png should it be able to build the correct URI.
Here goes the whole thingy:
Comment #94
jenlamptonTHREE TEMPLATES? this is just silly. no wonder its so hard to track down the problem. I'll take a stab at consolidating these and figuring out why the derivatives can't be generated.
Comment #95
jenlamptonFixed the problem. Re-running tests. Next patch will consolidate temlates.
Comment #96
jenlamptonthis patch removes the unnecessary picture-source.html.twig template. Will do one more tomorrow.
Comment #98
jenlamptonThis fixes the failing test, and cleans up some incorrect text in the tests. Going to consolidate in the next patch.
Comment #99
jenlamptonAnd here's the last consolidation, getting us down to one template for picture elements. woot!
Comment #101
RainbowArrayFun little four-hour patch that still needs further review.
Trying to correct a couple bugs in the patch on #99. Haven't been able to find the origin of the second bug. Trying again tomorrow.
Here's the patch and the interdiff with #99.
Comment #102
RainbowArrayFun little four-hour patch that still needs further review.
Trying to correct a couple bugs in the patch on #99. Haven't been able to find the origin of the second bug. Trying again tomorrow.
Here's the patch and the interdiff with #99.
Comment #103
RainbowArrayAccidentally hit save twice. Sorry. Marking as needs review so it's clearer which error remains.
Comment #104
RainbowArrayAlso, I should note that I manually set up mappings and followed the other instructions at the top of this issue, and I wasn't able to reproduce the error that testbot is finding. So there may be something wrong with the way the test is written.
Comment #106
joelpittet@mdrummond nice work getting rid of half the errors! Sorry it took 4 hours:S
Keep in mind some tests failures will happen in CLI but not in the web interface, which is kind of annoying but good to keep in mind when you're going crazy.
Comment #107
RainbowArraylarowlan did all the hard work of figuring out what was going wrong. I just set up the patch and botched up my git patch workflow. Anyhow, hopefully we'll figure out the rest soon.
Comment #108
larowlanThis should fix the fails.
The issue is that the new logic (in earlier patches) doesn't handle what happens when there is no picture mapping and therefore no breakpoints. The old code in theme_picture_formatter exited early by outputting the original image. This adds a second class of fallback if no sources exist, which is just the original image.
Comment #109
larowlanwoot green, thanks mdrummond, joelpittet
Comment #110
star-szrThanks everyone. Nice diffstat!
4 files changed, 157 insertions, 200 deletions.
Comment #111
star-szrTagging for reroll.
Comment #112
pplantinga CreditAttribution: pplantinga commentedreroll
Comment #113
joelpittetThis one is looking good, nice work. Untagging the re-roll.
Comment #114
gavin.hughes CreditAttribution: gavin.hughes commentedLooks good RTBC
Comment #115
alexpottPatch no longer applies.
Comment #116
pplantinga CreditAttribution: pplantinga commentedRe-roll.
Comment #117
pplantinga CreditAttribution: pplantinga commentedComment #118
pplantinga CreditAttribution: pplantinga commentedComment #119
Eric_A CreditAttribution: Eric_A commentedtravis-echidna, Cottser and jeanfei have been adding green test coverage for
theme_picture()
andtheme_picture_formatter()
(next to one red assertion) in the process of trying to target a specific bug. Since this issue is RTBC and the other is not I'll try to add this new test file here, and within the hour. (Just the green assertions of course.)Comment #120
Eric_A CreditAttribution: Eric_A commentedThis is #116 with tests added by travis-echidna, Cottser and jeanfei that capture some existing functionality. We'd expect both patches to be green.
Comment #122
pplantinga CreditAttribution: pplantinga commentedMaybe this will fare better?
Comment #124
pplantinga CreditAttribution: pplantinga commentedWorst patch ever. Here's a replacement.
Comment #126
jenlamptonIt looks like picture module may be removed from core, so postponing this issue until a decision has been made.
Comment #127
RainbowArrayWhere is the discussion taking place about picture's inclusion or removal from core?
Comment #128
attiks CreditAttribution: attiks commented#127 see #2120875: Remove breakpoint and picture module from core
Comment #129
webchickI don't feel like that issue is particularly close to resolution, and whether the module lives in core or contrib, it'll still need to be converted to Twig. :) Therefore, I think it's ok to continue working on this.
Comment #129.0
webchickUpdate commit message
Comment #130
cosmicdreams CreditAttribution: cosmicdreams commentedComment #131
joelpittet124: picture-1898442.124.patch queued for re-testing.
Comment #133
joelpittetComment #134
steinmb CreditAttribution: steinmb commentedChasing head.
Comment #136
yanniboi CreditAttribution: yanniboi commentedI'm just having a look at the errors being thrown by the last test.
"picture element in theme_picture() correctly renders with a NULL value for the alt option."
This is trying to be asserted:
$rendered_element at this point is '\<\i\m\g src="h\t\t\p://localhost/drupal/sites/default/files/simpletest/355953/image-test_0.png" />'
(ignore the '\'s in the above string, the text format on the comment was trying to evaluate the image tag and I could'y work out another way of making it take the html as a string...)
Not sure what the expected outcome here should be.
Comment #137
yanniboi CreditAttribution: yanniboi commented"img element in theme_picture() correctly renders the alt option."
and
"picture element in theme_picture() correctly renders the alt option."
Comment #138
yanniboi CreditAttribution: yanniboi commented"theme_picture_formatter() correctly renders without breakpoints and with a NULL value for the alt option."
and
"theme_picture_formatter() correctly renders without title, alt, or path options."
and
"theme_picture_formatter() correctly renders a link fragment."
$rendered_element is an empty string, so maybe the theme_picture_formatter function isn't working correctly.
Comment #139
yanniboi CreditAttribution: yanniboi commentedI think I have found why the tests are failing for the picture_formatter theme implementation.
theme_picture_formatter() is removed, but there is no preprocess function to replace it.
theme_picture() is replaced with template_preprocess_picture()
Comment #140
star-szr@yanniboi - thanks for taking a look! I believe #2009662: [REGRESSION] Replace theme() with drupal_render() in picture module is pretty relevant for the test coverage/failures, check the last handful of comments there.
Comment #141
Eric_A CreditAttribution: Eric_A commentedBecause of #2042773: Change #items within a theme_field() render array from an *array* to the same $items *object* used throughout the rest of the formatter pipeline the assertions added in #120 by travis-echidna, Cottser and jeanfei now need updating. They'll probably get simpler. These tests *could* be moved into their own issue *but* it would be wise to not simply ignore test coverage.
Comment #142
star-szrComment #143
star-szrTagging for reroll before we look at the tests again. If you start to look at the test changes please post the reroll first then an updated patch an interdiff. Thanks!
Comment #144
jayeshanandani CreditAttribution: jayeshanandani commentedUploaded a rerolled patch.
Comment #145
jayeshanandani CreditAttribution: jayeshanandani commentedComment #146
jayeshanandani CreditAttribution: jayeshanandani commentedComment #148
jayeshanandani CreditAttribution: jayeshanandani commentedComment #150
YesCT CreditAttribution: YesCT commented1.
the previous patch has some noise in it about newlines at the end of file. That should be fixed.
2.
$path is unused in testPictureTheme() and $expected_result is set, then not used, then reset.
so those should be taken out or asserts should use them.
3.
The test comment is:
and in the code:
but a call to theme_picture() is not in the function,
just
I can't find theme_picture() anywhere in core with this patch applied.
this patch renames it (replaces it?)
4.
in there:
5.
Also, I tried using minimal profile to test this... and it was problematic.
6.
So with standard profile, I used the steps to reproduce, and got to an image, which I inspected to find this html:
7.
I thought the tests might be failing because the alt tag was not part of
<picture>
anymore.(See http://picture.responsiveimages.org/ picture)
and that we needed to update the tests to not expect alt in
<picture>
but that html code shows that we are still outputting
<picture alt="">
8.
the tests have a situation where alt is unset.
a.
But in the ui, I dont see how we can unset it. alt is a field, and so even if that is not filled out, it would be an empty string?
b.
unsetting, that would have to be programmatically, right?
9.
Maybe if we found where render() was controlling the output, we could make it match http://picture.responsiveimages.org/ and fit the tests to that?
but I guess that might be a separate issue, since this is supposed to just be a conversion.
10.
the tests assume that if alt is unset that alt="" should be in the source, but if alt is null, that alt="" should not be printed. this is confusing for me.
I suppose it's supposed to read as: if alt is not set the default is alt="", if alt is set, but null, then it is intended not to be output at all.
this get's into tricky versions of == vs === and empty string vs null vs not set.
11.
I want to see what $rendered_element is actually in the test. what is the best way to do that?
xdebug, and set a breakpoint?
[edit:]
also tried:
but printing the redered_element gave an error:
Stopping on this for now.
Comment #151
RainbowArraySo I hate to rain on the parade of all the work that has been done on this issue, but looks as if part of this issue is about outputting an alt attribute on the picture element, but there shouldn't be an alt attribute on the picture element. The alt attribute goes on the img element inside of the picture element.
The specification for the picture element has changed in the last couple months, so it's possible alt was on picture in the past, but that's no longer the case.
Here's the current spec: http://picture.responsiveimages.org
Comment #152
jayeshanandani CreditAttribution: jayeshanandani commentedRemove unused variables and changed functions.
Comment #153
jayeshanandani CreditAttribution: jayeshanandani commentedRemove unused variables and changed functions.
Comment #156
jayeshanandani CreditAttribution: jayeshanandani commentedEnd line issue resolved.
Comment #158
YesCT CreditAttribution: YesCT commentedI think that interdiff was a diff of two patches? and not a git diff (looks weird in dreditor)
also seems to be more than just fixing the newlines at the end of files.
Fixes #150.3 also?
and,
huh? is that a paste mistake?
and there are some other chunks changed... more than just newline fixes.
I think we can leave this and focus on #2211831: Removal of alt attribute from [picture] tag first. Postponed on that.
When we come back here, we will probably need to redo this issue, but it will be simpler.
Comment #159
attiks CreditAttribution: attiks commentedFYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed
Comment #160
RainbowArray#2124377: Rename "Picture" module to "Responsive Image" module is in, and so is #2211831: Removal of alt attribute from [picture] tag, so this can be rerolled and simplified, since the alt attribute tests aren't needed any more.
Comment #161
jayeshanandani CreditAttribution: jayeshanandani commentedComment #162
jayeshanandani CreditAttribution: jayeshanandani commentedLot of things have been changed since the patch was rerolled. Looking into details for same.
#2009662: [REGRESSION] Replace theme() with drupal_render() in picture module has also got some relation to this. Looking at both issues and checking what all functions are needed to be changed.
Comment #163
jayeshanandani CreditAttribution: jayeshanandani commentedRerolled #152 since many functionalities got change after that (#162)
Comment #165
jayeshanandani CreditAttribution: jayeshanandani commentedRerolled #148 cleanly. Straight reroll without any modifications.
Comment #166
RainbowArrayI've been working with jayeshanandani, who's doing a great job on this issue. Walked through the reroll and found a couple other tweaks that were necessary. Then we'll start in on fixing the issues since #148.
Comment #169
jayeshanandani CreditAttribution: jayeshanandani commentedI've been working with mdrummond on this issue. We made a clean reroll and then looked out for issues in the same. This patch modifies some function fixes that were changed. There are some issues still left due to which image is not being rendered with picture and source tags. Working on the issue to get that fixed, in meantime uploading a minor fix.
Comment #171
jayeshanandani CreditAttribution: jayeshanandani commentedMade changes to #169 patch. Now image renders correclty with picture and source tag.
Remaining Tasks:
1. Get alt alttribute out from picture tag.
2. Rewrite tests for new functions.
Comment #172
star-szrI mentioned in IRC that it might make sense to rename picture.html.twig to responsive-image.html.twig, that was the main confusion here I think.
Thanks @jayeshanandani and @mdrummond for your hard work here!
Comment #173
webchickYeah, is there a reason we don't do that here? Because:
a) the patch as-is leaves us in the confusing world where there's both #theme => image and #theme => picture. The idea of renaming the module was to avoid that confusion.
b) "responsive-image.twig.html" would de-couple the concept "responsive images" from the specific implementation
<picture>
which may or may not change over the course of HTML5+. (It is admittedly a bit of a mouhtful, though)Comment #174
RainbowArray+1 to renaming picture.html.twig to responsive-image.html.twig.
We name templates after their purpose, not a specific HTML element. Makes sense.
Comment #175
RainbowArrayWe need to get this fixed before we can continue with the conversation: #2220865: Add empty img element inside picture element.
Comment #176
jayeshanandani CreditAttribution: jayeshanandani commentedComment #177
star-szrThis is blocked on #2220865: Add empty img element inside picture element then.
Comment #178
star-szrActually we can still move forward a fair bit on this instead of twiddling our thumbs until #2220865: Add empty img element inside picture element gets in. @jayeshanandani is working on this during core mentoring.
Comment #179
Eli-TComment #180
webchickThen another naïve question, which probably has nothing to do with this issue but just in case...
Why do you sometimes but not always want responsive images? Wouldn't we want all images to be responsive..?
Comment #181
jayeshanandani CreditAttribution: jayeshanandani commentedRemoved alt from picture tag and wrote new tests for twig conversion. Renamed the picture.html.twig to responsive-image.html.twig and other minor changes.
Comment #182
jayeshanandani CreditAttribution: jayeshanandani commentedDiff between #171 and #181.
Comment #184
jayeshanandani CreditAttribution: jayeshanandani commentedDiscussing this issue with @mdrummond and @YesCT in irc:
While going through past comments and reviewing patches we found that theme_picture, theme_picture_formatter and theme_picture_source got merged into template_preprocess_responsive_image() on #99. Later on #120 we found the tests coverage for theme_picture, theme_picture_formatter but not for theme_picture_source. Now since all 3 functions theme_picture, theme_picture_formatter and theme_picture_source were merged into one function ie: template_preprocess_responsive_image, we got confused on why was #120 adding tests for the functions that were being removed. We tried to understand and then there was a suggestion to update the tests made in #120 make work for template_preprocess_responsive_image. Modification of tests shall include tests coverage from #120 along with some modifications.Some of the things that needs to be changed in tests:
1. #120 Tests checks only for
<picture>
but not</picture>
. We need to make sure that we get both picture tags. Also there should be an<img>
tag inside<noscript>
as well as outside of that tag. The tests written in #120 doesnt covers that.2. We need to check the above tests for value of alt when set and unset at both times.
3. We need to include tests without breakpoints,alt and title attribute and check for same.
Comment #185
jayeshanandani CreditAttribution: jayeshanandani commentedNew patch adds missing tests for picture element. It covers all tests as discusses in #184.
The patch will fail the tests as there is no
<a>
tag being generated by template_preprocess_responsive_image. The old code used to generate<a>
on image elements.Patch also removes title attribute from
<picture>
tag.Comment #186
jayeshanandani CreditAttribution: jayeshanandani commentedComment #187
jayeshanandani CreditAttribution: jayeshanandani commentedMinor changes to ResponsiveImageThemetest file.
Comment #190
jayeshanandani CreditAttribution: jayeshanandani commentedPostponing the issue as #2123251: Improve DX of responsive images; convert theme functions to new #type element is a major issue. Once that gets in , this will need a reroll of patch along with some function change.
Comment #191
jayeshanandani CreditAttribution: jayeshanandani commentedComment #192
Wim Leers#2260061: Responsive image module does not support sizes/picture polyfill 2.2 got committed (YAY!), hence this is now unblocked :) But actually, that patch already performed the conversion. So marking this as a duplicate.
Comment #193
star-szrThanks Wim! :)
However it looks like theme_responsive_image_formatter() still exists, that should likely be converted still. Leaving postponed on #2123251: Improve DX of responsive images; convert theme functions to new #type element for now but I don't necessarily agree that this is blocked on that issue, at least not at this time.
Gave the issue summary a bit of an update.
Comment #194
attiks CreditAttribution: attiks commentedI had a quick look at the code, it is similar on how image does it, so why remove it in one place and not the other?
Comment #195
joelpittet@attiks what "other" are you referring to?
Comment #196
attiks CreditAttribution: attiks commented#195 theme_image_formatter
Comment #197
star-szrThat doesn't exist, unless you are proposing a consolidation with image-formatter.html.twig?
Comment #198
attiks CreditAttribution: attiks commented#197 You're right, I checked using
ag theme_image_formatter
, but the only results are inside comments.So we need to convert
theme_responsive_image_formatter
totemplate_preprocess_responsive_image_formatter
Comment #199
star-szr#2123251: Improve DX of responsive images; convert theme functions to new #type element hasn't really moved so re-activating this to convert theme_responsive_image_formatter().
Comment #200
attiks CreditAttribution: attiks commentedClosing this so it can fixed in the same commit of #2123251: Improve DX of responsive images; convert theme functions to new #type element, thanks all
Comment #201
star-szrThanks @attiks, it's great to see momentum there! Don't want to be a pessimist but postponing until that is actually committed because otherwise things can get lost.
Comment #202
attiks CreditAttribution: attiks commented@Cottser good thinking ;-)
Comment #203
RainbowArraySince #2123251: Improve DX of responsive images; convert theme functions to new #type element is in, this can officially be closed and joelpittet can finally check this conversion off his list!
Comment #204
star-szrWoohoo! Thank you @mdrummond :)