Various subclasses of views_object define a pre_render method. Some pass by reference, allowing pre_render to change what is being rendered. Others pass by value. If the subclass then is declared by reference, an E_STRICT notice is thrown.
In my specific need, views crosstab needs to add a summary row to the query results.
This implied that views_plugin_style and views_plugin_row both be changed to pass by reference.
The issue that I see is that while a single patch can update views, other modules which subclass these objects will then need to be updated as well.
This should somehow be ported to views in core (assuming it still has pre_render functions) for D8. I'm not sure how that works when a module is absorbed into core.
List of related modules which will need updating after this patch is committed:
- References: references_plugin_row_fields::pre_render()
Comment | File | Size | Author |
---|---|---|---|
#2 | views-pre_render_reference_parameter-2290127-2.patch | 4.37 KB | DanChadwick |
Comments
Comment #1
dawehnerWell, in this case I recommend you to write a patch and hope that breaking all the other instances isn't a big problem.
Comment #2
DanChadwick CreditAttribution: DanChadwick commentedSuper! I'm delighted that you are willing commit the patch. This patch changes all pre_render functions within views to use a reference parameter. Those with E_STRICT (which is by default on in PHP 5.4, but may easily be turned off) will notice that they should adjust their declarations.
Comment #3
DanChadwick CreditAttribution: DanChadwick commentedComment #4
DanChadwick CreditAttribution: DanChadwick commentedComment #5
DanChadwick CreditAttribution: DanChadwick commentedSo I guess #1 was sarcasm. My apologies; I misunderstood.
Comment #6
dawehnerPlease calm down. You know, time is simply limited and there are hundreds if not thousands of issues in my tracker.
You should totally know that feeling :)
In general its more tricky to commit this, than you think it is. If we commit this, what do we do in Drupal 8? Should we just drop the reference there again?
Technically adding the reference now is an API break in 8.0.x and even 8.x.y, so strictly speaking we cannot add it in 8.x at all. Is this okay? I don't know.
Comment #7
DanChadwick CreditAttribution: DanChadwick commentedThe apology was sincere. I get that it's a cluster to fix this at this point, particularly now that D8 is out.
Comment #8
dawehner@DanChadwick
Would you commit this patch with the knowledge that it might be hard to get the patch into D8?
Comment #9
DanChadwick CreditAttribution: DanChadwick commented@daweher - Assuming that's not a rhetorical question, I would because:
a) It's the right signature. It is fixing a long-standing inconsistency in the API, which results in a lack of flexibility.
b) Right now, it's only a strict message. After PHP7 it will be a warning message. So better now before PHP7 is supported by D8. An in fact, it might be fixed as part of the PHP7 support. :) http://php.net/manual/en/migration70.incompatible.php
c) I think very few modules are affected.
Comment #11
dawehnerNo not all a rhetorical question. Do you mind fighting this with the core committers?
Comment #12
dawehnerComment #13
kristiaanvandeneyndeWell, I just got an Entity API bug report in my issue queue because of this :)
#2636912: views: rendering entity throws strict warning
Judging by #1 that may just be the tip of the iceberg.
Do I ask fago to fix this in Entity API or should I hold my horses in case this gets reverted?
Comment #14
dawehnerWell, this is not much of a surprise to be honest :)
Comment #15
maximpodorov CreditAttribution: maximpodorov commentedMany modules are affected: BEF, Entity API, Entity Reference, Date iCal, ...
Comment #16
aDarkling CreditAttribution: aDarkling as a volunteer commentedI agree that consistency needs to be enforced now before this becomes yet another pervasive minor issue later on.
The number of affected modules will just keep growing is this isn't handled now. There's no upside to waiting.
Comment #17
maximpodorov CreditAttribution: maximpodorov commentedSo this change should be reverted in 7.x since D7 projects must be stable now.
Comment #18
dawehnerWell, do you mind fighting this with https://www.drupal.org/u/danchadwick ...
Comment #19
maximpodorov CreditAttribution: maximpodorov commentedI think such breaking changes are are only possible in Views 7.x-4.x, not in 7.x-3.x.
Comment #20
dawehnerI agree, its not worth the pain. Reverted the original patch.
Comment #21
andypostSuppose views should have API "to add a summary row to the query results" but
here's a inconsistency only in D7!
D8 defines this method in
*PluginBase
classes and does not use reference, so probably could be changed in minor releases by introducing "renderable" plugins but that separate issueComment #22
andypostComment #23
donquixote CreditAttribution: donquixote commentedJust for the record, here is another case where this caused disruption:
#2652990: Strict warning: Declaration of entityreference_plugin_row_fields::pre_render() should be compatible with views_plugin_row::pre_render(&$result) in _registry_check_code() (line 36 of entityreference_plugin_row_fields.inc).