Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
when using an image field and display it on a view and some image fields are empty then we get the following notice
Notice: Undefined property: views_handler_field_field::$original_value in views_handler_field->render_text() (line 1118 of sites\all\modules\views\handlers\views_handler_field.inc).
when digging and debugging code I realized that in
function render_text($alter) {
$value = $this->last_render;
if (!empty($alter['alter_text']) && $alter['text'] !== '') {
$tokens = $this->get_render_tokens($alter);
$value = $this->render_altered($alter, $tokens);
}
if (!empty($this->options['alter']['trim_whitespace'])) {
$value = trim($value);
}
// Check if there should be no further rewrite for empty values.
$no_rewrite_for_empty = $this->options['hide_alter_empty'] && $this->is_value_empty($this->original_value, $this->options['empty_zero']);
this->original_value is undefined for empty images thus if I put a check and fill in the original value right before calling is_value_empty then all notices go away, I did this
if(!isset($this->original_value))
$this->original_value = null;
// Check if there should be no further rewrite for empty values.
$no_rewrite_for_empty = $this->options['hide_alter_empty'] && $this->is_value_empty($this->original_value, $this->options['empty_zero']);
for anyone experiencing the same this is a dirty hack, if I knew the proper way to solve this I would provide a patch
Comment | File | Size | Author |
---|---|---|---|
#59 | views-1548240-60.patch | 5.69 KB | tim.plunkett |
#47 | 1548240-test-only.patch | 4.21 KB | dawehner |
#47 | 1548240-not-only-tests.patch | 5.37 KB | dawehner |
#44 | views-1548240-44.patch | 2.5 KB | tim.plunkett |
#19 | views-1548240-19-tests.patch | 1.5 KB | tim.plunkett |
Comments
Comment #1
3rdLOF CreditAttribution: 3rdLOF commentedI have this problem with my commerce views. I wonder if this is somehow tied to the problem I am having with links that use "destination" in commerce not working properly.
Comment #2
fgmAfter pulling the latest dev today, I now have this problem too for any empty field. Observed on text and file (generic file) fields.
Comment #3
fgmPatch based on #1. Fixed problem and field handler tests pass locally with it.
Comment #4
fgmOops wrong patch.
Of course, you could argue that after the change isset($this->original_value) will still fail, but this seems to actually be a quality of the patch, not a default, as it impacts behavior the least, whereas using property_exists($this, 'original_value') would be slightly less stealth.
Comment #5
phani00 CreditAttribution: phani00 commentedthank you, works for me.
Comment #6
dawehnerIt would be helpful to explain in a short comment this this variable might not be instanciated ... Alternative what about documenting the variable on the class level?
Comment #7
fgmActually, although this patch is minimal and answers the need well, I'm not fond of it, as it essentially appears to cover up an error higher up in the view build process, but I did not see where original_value should have been set in the first place, although this is probably where the problem should be fixed, instead of just at the point of use of the value, precisely because there could be other points of use and fixing in one place won't fix the others. Maybe you could point out where this comes from normally ?
Comment #8
fgmCaused by the changes in #1273486: “Empty Text” does not work when “Hide Rewriting if field is empty" option is selected..
Comment #9
fgmThis version is just as short but much cleaner IMHO.
Comment #10
Macronomicus CreditAttribution: Macronomicus commented#9 Seems to have it all sorted.
Cheers!
Comment #11
fgmNot really, dawehner mentioned on IRC that the field handler is used on each row, so by only initializing originalValue at handler construction, a valid value for one row won't be reinitialized for the next and can still cause the original error if the field is empty on the next row. That really needs a test case to qualify the next patch.
Comment #12
dawehnerSo adding some tags
Comment #13
robertstaddon CreditAttribution: robertstaddon commentedThe patch in #9 resolved the error on my site.
Comment #14
msamavi CreditAttribution: msamavi commentedPatches could not help!
Comment #15
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedpatch in #9 worked for me
Comment #16
3rdLOF CreditAttribution: 3rdLOF commentedPathed in 9 does not seem to ve working in latest dev
Comment #18
tim.plunkettWorking on a test.
Comment #19
tim.plunkettOkay, here's a test and a fix.
Comment #20
fgmThe reason why my earlier patch was not good enough (AIUI) is the situation of a test for which the first result row for a field is not empty, but one of the later rows is empty, which needs two nodes, the first of which has a value on the tested field and the other does not. I do not think your test as currently done would catch this.
Comment #21
tim.plunkettFeel free to expand the test coverage.
EDIT
I say this only because I'm not 100% sure myself what to really test for.
Comment #22
damiankloip CreditAttribution: damiankloip commentedRTBC for this fix, as I just came across this error, did a pretty much similar fix, then remembered this issue :)
Comment #23
dawehnerWhat happened with the documentation on the class level?
Additional: I somehow share the comment of @fgm, because there some cases which aren't covered by the tests :(
Comment #24
damiankloip CreditAttribution: damiankloip commenteddawehner, do you mean what's in the patch in #9?
Comment #25
drupert55 CreditAttribution: drupert55 commentedHello.
I saw the error in the previous dev version and the newest dev version (2012-May-17) when in Views: Format Slideshow => view page.
Comment #26
blackandcode CreditAttribution: blackandcode commented#19: views-1548240-19-combined.patch queued for re-testing.
Comment #27
damiankloip CreditAttribution: damiankloip commentedNot sure this needs to be re tested again?
Comment #28
somatics CreditAttribution: somatics commentedHas this or is this going to be rolled into a Dev release?
I need this patch, but I would like to know whether or not it's going to be in the regular module codebase -- as I am regularly updating Views to the latest Dev version as they are released.
Comment #29
damiankloip CreditAttribution: damiankloip commentedIt will be added when the issue is marked as fixed and it says its been committed.
Comment #30
somatics CreditAttribution: somatics commentedOkay, thanks; can you clarify what that means? Do you mean that the patch needs further testing before it can be declared a fix for the issue? Or are there some other processes that need to occur before the issue is marked fixed?
Comment #31
astutonetHi.
I also have this issue.
The combined.patch in #19 solved my problems.
Tks.
Comment #32
bayousoft CreditAttribution: bayousoft commented#19 fixed the error in my case
Comment #33
dawehnerAssign to myself to write a bit more tests
Comment #34
WTFranklin CreditAttribution: WTFranklin commentedJust wanted to comment and say the patch from #19 worked for me as well
Comment #35
KingSalibah CreditAttribution: KingSalibah commentedPatch worked for me too. This really should be added quickly to the latest dev version, IMO.
Comment #36
drupalycious CreditAttribution: drupalycious commentedI agree with KingSalibah, the patch should be added, otherwise each time views is updated (I use the dev version) the patch has to be applied.
thanks
Comment #37
mxhalso confirm #19 that it works with current dev from June 1st. This must be commited.
Comment #38
johnvwell, everyone agrees..
Comment #39
damiankloip CreditAttribution: damiankloip commentedIt's all good and well everyone saying "..Must be committed" etc.. But no one is honouring dawehners comment in #33, that more tests are needed. If people want this to be committed they should try and help with tests, rather than just going with the just do it now approach.
Comment #40
drupalycious CreditAttribution: drupalycious commentedwell I would like to help with "tests" but I am a newbie for what concerns the code of the modules.
I don't know about which kind of tests you are speaking and what is needed to do.
Practically I apply the dev module plus the patch and I test it on my website, and come back here to report that it has indeed fixed the issue I had.
Thanks for the module and patchs!
Comment #41
tim.plunkettThe failing patch in #19 contains a test for the broken behavior I was seeing. It proves that the passing patch in #19 fixes that particular bug. fgm and dawehner are suggesting that there are other cases in which this is broken, and that those cases also need tests.
Comment #42
webadpro CreditAttribution: webadpro commentedHas this patch been commited yet?
Comment #43
damiankloip CreditAttribution: damiankloip commentedThe issue will be marked fixed, if/when this is committed.
Comment #44
tim.plunkettRerolled #19 + #9
Comment #45
dawehnerThanks for the new patch! Commmited it to both 7.x-3.x and 8.x
New tests are coming
Comment #46
damiankloip CreditAttribution: damiankloip commentedYay!
Comment #47
dawehnerThis patch shows that there is still another bug involved.
Comment #48
tim.plunkettSo, the test only patch didn't fail. So something is wrong.
Comment #49
webadpro CreditAttribution: webadpro commentedisnt it just me, or hasnt both passed the test?
Comment #50
tim.plunkett@webadpro both passed the test. Which is wrong.
A new test, without a fix, should fail. It proves that something is broken. Then, when combined with the fix, it should pass.
This means that either the test is flawed and isn't proving the bug exists, or that the bug just doesn't exist.
Comment #51
webadpro CreditAttribution: webadpro commentedOh, I hear you. :) Makes Sense.
Comment #52
dawehnerActually i'm somehow convinced now that there is another bug i don't understand yet.
$this->last_render will be get a reset on every row, so maybe we could just get in the test.
Comment #53
KingSalibah CreditAttribution: KingSalibah commentedI am not sure if this is related, but occasionally, since using the latest dev version, I have been getting various notice errors, but it still populates the data underneath the error notices. I have some interesting views contextual filters which I have been playing with. Normally I figure these out. But I just got one this morning, so I'll see if I can explain it and then perhaps you can tell me if it is related.
I have a node type (let's call is "cars") - one node per car brand. I have a block visible on the sidebar which has a contextual filter saying if the node type = say "BMW" then countDistinct all BMW models in stock. Let's say there are 7. This is a link to a page for the stock. The link retakes the node ID and uses that in the contextual filter for the page, so it pulls just the list of BMWs in stock. The page lists the BMWs in stock. This all works. All using views (no special coding).
Then I have exposed filters. Now, when I choose and apply a filter I get a notice error for each item returned. The error I get is "Notice: Array to string conversion in views_handler_field->get_render_tokens() (line 1385 of /serverpath...../sites/all/modules/views/handlers/views_handler_field.inc)." I get this error for each item returned, once a filter is applied. Related? Is it possible that the brand type is getting lost, creating this error?
Comment #54
KingSalibah CreditAttribution: KingSalibah commentedI see my issue was solved at http://drupal.org/node/1566770#comment-6053584
Comment #55
mikemadison CreditAttribution: mikemadison commented#19 seems to have resolved this for me.
Comment #56
GiorgosKupdating to latest dev of views and ctools seems to have resolved this for me
can anybody else confirm this ?
Comment #57
johnvI am confused, too. I did not apply this patch after my latest update, and error does not show up anymore.
Patches in #44, #47 contain extra tests, are they still relevant?
Comment #58
dawehnerThe main bug is fixed but i thought of another possible one, but as the patch failed the status is wrong.
Comment #59
tim.plunkettI was able to replicate this bug, and found another. I didn't yet update the test, just posting what I have.
Comment #60
frobpatch applies cleanly, any more movement on this?
switching status for test-bot and unassigning as it has been over a year now.