Currently we have a conditional in the Style plugin that checks the field alias, and only displays the rendered field value if it's a unknown alias. This is a bit backwards, we can simplfy this logic by ALWAYS calling render() on the field value but add an option to the row options (to go with the alias option) to output the raw values. This can then be checked when render time comes around, and the raw/rendered output will depend on this and not if the alias is unknown or not.
This patch also creates a table for the row plugin options instead, imo this is much better. I have also added some more tests for this.
I'm looking for feedback on what we need to label these are for the table etc... as well as reviews obviously :)
Comment | File | Size | Author |
---|---|---|---|
#15 | 1929014-15.patch | 12.35 KB | damiankloip |
#15 | interdiff.txt | 654 bytes | damiankloip |
#13 | 1929014-13.patch | 12.35 KB | damiankloip |
#13 | interdiff.txt | 665 bytes | damiankloip |
#9 | 1929014-9.patch | 12.36 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedSo some initial thoughts from the code side:
Comment #2
damiankloip CreditAttribution: damiankloip commentedSorry, this patch, the first one has some changes to FieldPluginBase that should have been removed from the patch.
I will also remove the dupe comments in the new tests in the next patch.
Comment #3
damiankloip CreditAttribution: damiankloip commentedComment #4
damiankloip CreditAttribution: damiankloip commentedSorry, Ignore that, posted on the wrong issue! #2 is good
Comment #6
dawehnerNice work!
What about simply rawOutputOptions?
Nice method!
I'm totally fine with making sanitizeValue a public method.
Additional you probably want to use $field->getValue ?
I'm wondering whether this should be protected instead? I don't see so much use-case for other people here?
Is there a reason why we use two different displays (rest_export_1 for the UI and ws_endpoint_1 for the test?)
Oh good to know that you actually want to have that option on the row plugin?
Can't we still have that option on the row plugin? It feels a lot like that would confuse people. Additional, can you think of use-cases where you really want to have this behavior, as the user don't have any clue what the "raw" output could be, and even worse, we probably can't provide one for fieldapi fields. I'm sorry this changed was probably not intendend
Comment #7
damiankloip CreditAttribution: damiankloip commentedThanks for the review! I have added your points to the patch.
The ws_endpoint_1 display doesn't actually exist anymore, that's actually a mistake from earlier iterations of the rest export patch :/ Which leads me to look at the logic in ViewExecutabel::setDisplay().... I think we should change that slightly.
Comment #8
dawehnerThere are just these two small issues.
Any reason to use format_string without variables? In general I'm wondering whether it really makes sense to compare against $view->result or should better just do one single $this->assertEqual($this->drupalGetAJAX() ... , $expected))
We can put the "get" into the previous line.
Any reason to use format_string without variables? In general I'm wondering whether it really makes sense to compare against $view->result or should better just do one single $this->assertEqual($this->drupalGetAJAX() ... , $expected))
Comment #9
damiankloip CreditAttribution: damiankloip commentedThanks for the review! I have made those changes, except for changing how the test works. It makes sense to test this by iterating as we have different keys v array/object to test against, so it may be more confusing to get an array of both values first then check?
Comment #10
dawehnerNice refactoring and better and more tests, nice!
Comment #11
xjm#9: 1929014-9.patch queued for re-testing.
Comment #12
catchCan't this be isset()? If the array value is NULL, it'll end up returning NULL either way.
Comment #13
damiankloip CreditAttribution: damiankloip commentedYes, I guess you will get the same outcome either way :) and isset() is more performant anyway.
Comment #14
dawehnerI don't think this is the right code :) Shouldn't it be isset($item[$key])?
Comment #15
damiankloip CreditAttribution: damiankloip commentedHA, yep ....
Comment #16
dawehnerWell in fact isset($a, $b, $c) works, oh php :)
Comment #17
damiankloip CreditAttribution: damiankloip commentedYeah, it will 'work'. Doesn't help us at all here though. You have got to love php for these things :)
Comment #18
catchThanks! Committed/pushed to 8.x.
Comment #19.0
(not verified) CreditAttribution: commentedUpdated issue summary.