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.Sorry!
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 834842.patch | 6.8 KB | Damien Tournoud |
| #9 | 834842.patch | 6.72 KB | Damien Tournoud |
| #7 | 834842.patch | 4.61 KB | Damien Tournoud |
| execute_callback_opsie.patch | 572 bytes | chx | |
Comments
Comment #1
webchickTests?
Comment #2
webchickComment #3
chx CreditAttribution: chx commentedForget it, thanks! :) You want me to
writeput another storage engine into core to test this?Edit: obviously now I have another storage engine which made me find this bug and believe me, I would be happy to add the mongodb storage engine into core just to test this ;) but I somewhat think that might meet a little resistance here and there.
Comment #4
yched CreditAttribution: yched commentedNot sure if that's relevant, but we do have a test storage engine in core : modules/field/tests/field_test.module / field_test.storage.inc
Comment #5
chx CreditAttribution: chx commentedThen just bury this patch as so many was buried before and make the subsystem unusable. I don't care. I won't write a 100 LoC patch just to write the storage query for that storage. It's not impossible it's just an incredible lot of work for nothing. It's hard to describe the frustration one feels when a trivial oneliner can't get in because "it needs tests"! Noone wanted tests for this feature when the original went in, why the fuss now?
Comment #6
jhodgdonThis fix doesn't look like it requires a test. It's a simple matter of making the code correctly call $function = $this->executeCallback;
to make sure that the executeCallback function is what is executed, rather than the literal member function executeCallback(). This is a no brainer. +1 on RTBC.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedI actually agree that this needs test.
Here.
Comment #8
chx CreditAttribution: chx commentedSure. But then please add inline comment for if (function_exists($this->executeCallback)) in the new method and I think return $storage . '_field_storage_query'; needs a coment too. Also if you are hell bent on testing this you need a module which alters the query setting executeCallback
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedSame patch with more comments, and a test for executeCallback in hook_entity_query_alter().
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedEven more inline comments (from chx).
Comment #11
chx CreditAttribution: chx commentedThat's nice :) (and it adds 94 LoC. I was not that far off by saying this takes 100 LoC. Thanks Damien.)
Comment #12
webchickYes, and 20 of those lines are further improvements based on things found by writing tests. ;)
Thanks, great work! Committed to HEAD!
This looks like it changes APIs. Could someone please write a couple lines that summarize the changes for modules implementing field queries?
Comment #13
webchickComment #14
jhodgdonActually, I don't think it changes APIs. It just makes it so that the execute() method actually does what was intended (and previously documented) for executeCallback() instead of having a broken executeCallback().
Tentatively marking back to Fixed, but feel free to set back to CNW if I'm wrong about that.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedAgreed, there is no API change here.
Comment #16
webchickCool. Sorry, my mistake.