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.
Currently clear_fields just exists on the sql query plugin, but it's called on more or less generic code.
Comment | File | Size | Author |
---|---|---|---|
#45 | 1933286-44.patch | 2.54 KB | Lendude |
| |||
#45 | interdiff-1933286-43-44.txt | 449 bytes | Lendude |
#44 | 1933286-43.patch | 2.56 KB | Lendude |
#44 | interdiff-1933286-42-43.txt | 451 bytes | Lendude |
#42 | 1933286-42.patch | 2.56 KB | Lendude |
Comments
Comment #1
dawehnerAdding some tests for add_fields, so you can be sure the method works as expected.
Comment #2
dawehner#1: drupal-1933286-1.patch queued for re-testing.
Comment #4
dawehnerComment #5
reszlipatch does not apply anymore, rerolling...
Comment #6
reszliattached is the rerolled patch
the function was already renamed from clear_fields into clearFields,
but it still needed the other changes
Comment #8
reszlipatch now applies correctly, but the new test introduces needs a complete rewrite
Comment #9
reszlilet's see if this fixes the problem with the test
Comment #10
reszliforgot to attach the interdiff
Comment #11
reszliComment #13
reszliattached new patch
fixed tests, they succeded locally
Comment #14
reszliComment #15
jhedstromComment #16
rpayanmComment #18
rpayanmComment #19
jhedstromWe should use
{@inheritdoc}
instead of the overrides bit.Needs a line break here.
Comment #20
jhedstromNote that
{@inheritdoc}
could just be added to the existing docblock comment, since specifying what the method is doing seems valuable.Comment #21
rpayanmComment #25
dawehnermhhh ... so
QueryPluginBase
does not have the notion of fields at all, at the moment.So I wonder whether it really makes sense to have such a generic method on there.
Comment #27
dawehnerAt that point its a 8.1.x thing, this is for sure.
Comment #31
dawehnerLet's use
{@inheritdoc}
, but also ensure that we don't change the second instance ... this is not needed to fix this particular issue, or actually is kinda more unrelated.Comment #42
LendudePer #25, I agree that we shouldn't add this to the base class, since that has no concept of fields.
So changing this to a task to just update the doc block and add test coverage for the clearFields method because that is still relevant.
Updated the patch in #21:
* Removed the changes to QueryPluginBase
* Updated the test to run on PHPUnit
No interdiff cause that wouldn't be useful, everything has changed.
Comment #43
LendudeThis came up as a bug smash initiative daily target, so tagging it as such
Comment #44
LendudeFixed the namespace for the test
Comment #45
LendudeAnd added a @group...
Comment #47
smustgrave CreditAttribution: smustgrave at Mobomo commentedRunning for D10
Comment #48
smustgrave CreditAttribution: smustgrave at Mobomo commentedDocument changes look good and (I think) would pass short doc when that lands.
Adding test coverage seems like a welcomed addition.
Code looks good not sure what else to check.