Problem/Motivation
When using the theme_image_formatter() function, the title setting in D7 and D8, and the alt setting in D7, are assumed to exist, but are not documented as required. In addition, the path options array is assumed to exist, and you can't pass the path options array without a path, which prevents a # fragment-only path.
Proposed resolution
Check to see if the title and options are set before using them, and use isset instead of !empty for the path.
Remaining tasks
Review and commit D8 patch, reroll for D7 since #1025796: Rename path to uri in image functions was D8-only.
User interface changes
n/a
API changes
n/a
Original report by Chris Gillis
theme_image_formatter() has the ability to wrap the image in a link. It uses the l() function which takes a second optional argument of 'options'. Problem is that theme_image_formatter() doesn't recognise that 'options' in optional, and treats it as required.
Line 551 of image.field.inc should change from:
$options = $variables['path']['options'];
to:
$options = !empty($variables['path']['options'])?$variables['path']['options']:array();
Sorry no patch.
Comment | File | Size | Author |
---|---|---|---|
#53 | drupal-1038932-53.patch | 3.66 KB | tim.plunkett |
#51 | drupal-1038932-51.patch | 3.66 KB | tim.plunkett |
#47 | drupal-1038932-47.patch | 3.69 KB | tim.plunkett |
#47 | interdiff.txt | 765 bytes | tim.plunkett |
#43 | drupal-1038932-43-tests.patch | 3.19 KB | tim.plunkett |
Comments
Comment #1
Chris Gillis CreditAttribution: Chris Gillis commentedPatch Attached
Comment #2
mr.baileysGood catch:
...will generate a notice.
I think it's preferred to use isset rather than empty in this case. Additionally, Drupal coding guidelines require spaces before and after the operators (see http://drupal.org/coding-standards#operators).
Comment #3
Chris Gillis CreditAttribution: Chris Gillis commentedThanks for the feedback. Patch updated.
Comment #4
tim.plunkettComment #5
tim.plunkettAlso included #1587870: Undefined index: alt in theme_image_formatter() and Undefined index: title in theme_image_formatter() and #1396164: theme_image_formatter() should check for array key.
Needs a bit more work, and a summary.
Comment #7
tim.plunkettI think I need $base_url in there or something.
Comment #10
tim.plunkettHm, can't reproduce locally, must be some prefix issue.
Comment #12
tim.plunkettAh!
Hopefully this works. Leaving the debug in for now.
Comment #13
tim.plunkettOkay! Here's those patches again, cleaned up, without debugging info.
Once these are in for D8 I will rewrite them for D7.
Comment #14
tim.plunkettAdded issue summary.
Comment #15
tim.plunkettComment #16
tim.plunkettRenamed the class and fixed the assertion messages. Finally ready by my standards.
Comment #17
tim.plunkettJust to expedite things.
Comment #18
xjmMostly this looks great to me.
Should we add any inline comments in here?
Can we add an article here? "...correctly renders a link fragment."
Comment #19
tim.plunkettThe title hunk already has a comment, and I added one for the path/options section that I think covers it.
And yeah, an article is a good idea there :)
Comment #20
jthorson CreditAttribution: jthorson commentedMinor nitpick. At first, I read the test comment as
instead of
Not sure if something like this would be clearer:
But as I said, this is a minor nitpick at most ... so bumping to RTBC.
Comment #21
tim.plunkettSince #1591434: Convert image tests to PSR-0 is about to move all the image tests around, here's a reroll using a PSR-0 test. Can be committed before or after that other issue.
Comment #23
tim.plunkett#21: drupal-1038932-21.patch queued for re-testing.
Comment #24
tim.plunkettThe image tests are PSR-0 now, so I rerolled to make sure one last time. I touched up the comment according to #20, and fixed the test class name.
Comment #25
cosmicdreams CreditAttribution: cosmicdreams commentedwhy can't empty() be used here?
Seems like a good opprotunity to use the ternary operator, ?:
http://www.php.net/manual/en/language.operators.comparison.php#language....
Comment #26
tim.plunkett0 is a valid title, no?
AFAIK, you can't use that ternary with isset(), or else the middle case becomes the result of it. It'd be like saying
$options = isset($variables['path']['options']) ? (bool) $variables['path']['options'] : array();
Comment #27
cosmicdreams CreditAttribution: cosmicdreams commentedAh you're right. Thanks for helping me see that. RTBC
Comment #28
leanderl CreditAttribution: leanderl commentedHello! I seem to have run into this problem (Drupal 7.14) and found this issue. What branch should I clone into to apply the patch? I tried git clone --recursive --branch 7.x http://git.drupal.org/project/drupal.git. I feel uncertain about how to patch drupal core.
Comment #29
jthorson CreditAttribution: jthorson commentedleanderl,
You can try downloading the 'combined' patch in #24 and applying it to your 7.x branch (using git apply) ... but the patch has not made it into the 7.x codebase yet. (The 'reviewed & tested by the community' status indicates that it's waiting for a core committer to add it ... once you see someone post a 'committed' comment here, then it will be included in a new clone.)
Comment #30
leanderl CreditAttribution: leanderl commentedThx for quick and constructive reply jthorson!
Comment #31
webchickThis looks like sensible hardening, and comes with a test. Woo!
Committed and pushed to 8x. This will need a re-roll for D7 thanks to PSR-0.
Comment #32
tim.plunkettRerolled.
Comment #34
tim.plunkettAh yes, this is where D7 and D8 differ. (they set 'alt' completely differently)
Comment #36
tim.plunkettAh, RDF module is on during D7 tests, and clean URLs are off.
This should finally be good to go.
Comment #37
dags CreditAttribution: dags commentedApplied and tested against an up to date 7.x branch and compared to the D8 version. Looks good.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commented#36: drupal-1038932-36.patch queued for re-testing.
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedI'm retesting this because I expect it to fail... the fact that clean URLs are off during D7 tests was a recent bug/regression (it was like that for a while in Drupal 8 too), now fixed in both places due to #1541958: Split setUp() into specific sub-methods).
So the test fails for me locally, and I'm guessing it will fail here too, unfortunately.
Also, we might not want to make any assumptions about clean URLs (one way or another) because I think that depends on the environment the test is running in? Perhaps it would be better to change the tests so that they use Xpath to look for the specific attributes they're asserting, rather than looking to match an entire string of HTML?
Comment #40
tim.plunkettIf it fails, please move it back to D8 so I can switch to xpath.
If it doesn't, then COMMIT IT! :D
Comment #41
David_Rothstein CreditAttribution: David_Rothstein commentedWell, it didn't fail, but it should have :) The testbot must be running with clean URLs off or something like that.
Because I tried it again locally, and the patch definitely failed for me when I ran it with clean URLs on, but turning clean URLs off and running the test again makes it pass (see test output below).
If this were a critical bug or something I might say let's commit it and worry about it later :) But since it's not, I think we should stabilize the tests first... sorry. Otherwise most people trying to run these locally are going to wind up getting failures, even if the testbot doesn't.
Comment #42
jthorson CreditAttribution: jthorson commentedJust confirming that the bot does indeed run with clean URLs off.
Incidently, this is the first time that has caused an unexplained 'pass' instead of an unexplained 'failure'! ;)
Comment #43
tim.plunkettWell I know this needs to be fixed in D8, but there is also the need to backport #1696416: Regression: theme_image_style() is broken, and I will have to figure out a way to do this all without making too much work.
One option is to commit this as is, and then open a major follow-up to fix the D8 tests to use xpath, and then backport the theme_image_style tests and the xpath fixes at once.
Switching back to D7 for now just to run the bot.
Comment #44
tim.plunkettActually the "parse exact HTML to prove theme functions work" is systemic, I really do think an xpath follow up is best.
Comment #45
tim.plunkett#1175764: Have theme('image_style') inject the style name as a class is a good example of what I mean.
Comment #46
tim.plunkettI opened #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output, I think #36 should be committed as is.
Comment #47
tim.plunkettThis is how other parts of core work around this issue.
Comment #48
tim.plunkettThis should probably switch to #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output
Comment #49
cweagansI don't have any objection to the patch in #47. Using it on a client project and it does what it's supposed to (we were getting some notices emitted because file_entity wasn't passing a title or alt attribute to the theme function and with the patch, that is no longer the case).
The code looks pretty straightforward, so I say RTBC.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedI was about to commit this but then noticed there's a subtle behavior change here.
Before the patch, you could explicitly do:
in order to force the "alt" attribute not to be printed in the HTML at all. After the patch, though, the "alt" attribute will still be printed, but as an empty string.
That doesn't sound like too big of a deal to me, except that the documentation for theme_image() (which theme_image_formatter() ultimately calls) goes to great lengths to explain that this option is available. So perhaps we don't want to break it?
In any case, preserving the old behavior might be as simple as changing this code to use array_key_exists() instead:
Drupal 8 isn't affected by this, just Drupal 7.
Comment #51
tim.plunkettGood point. Updated the patch.
Comment #53
tim.plunkettI clearly forgot how array_key_exists works.
Comment #54
tim.plunkett#53: drupal-1038932-53.patch queued for re-testing.
Comment #55
tim.plunkettThis addressed #50, and still passes. RTBC anyone?
Comment #56
attiks CreditAttribution: attiks commentedLooks good, slipped my mind
Comment #57
webchickI'm confused on whether this needs to be held up on #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output or not.
Comment #58
webchickTim and I talked, and he pointed out that there are many places that could use the new assertThemeOutput(), so doing that more core-wide could be a follow-up, rather than a blocker here. I'm inclined to agree.
Committed and pushed to 7.x. Thanks!
Comment #59.0
(not verified) CreditAttribution: commentedadded issue summary