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.
We need to rename properties according core standards in views/Plugin/views/filter/InOperator.php
Part of meta-issue #2052421: [META] Rename Views properties to core standards
Comment | File | Size | Author |
---|---|---|---|
#38 | interdiff-2183421-35-38.txt | 1.09 KB | alimac |
#38 | in_inoperator_rename-2183421-38.patch | 17.31 KB | alimac |
#35 | interdiff-2183421-33-35.txt | 1.51 KB | alimac |
#35 | in_inoperator_rename-2183421-35.patch | 18.41 KB | alimac |
#33 | interdiff-2183421-31-33.txt | 941 bytes | alimac |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #4
InternetDevels CreditAttribution: InternetDevels commentedNew patch attached.
Comment #6
sidharthapUpdating issue tag. Will have a look at Drupal Camp Mumbai sprint on this weekend.
Comment #7
Crisz CreditAttribution: Crisz commentedComment #8
Crisz CreditAttribution: Crisz commentedComment #10
Crisz CreditAttribution: Crisz commentedStill working on it.
Comment #11
alimac CreditAttribution: alimac commentedThe patch in 10 refers to pre-PSR directories (lib/Drupal/ instead of src/). I was going to reroll the patch per instructions in https://groups.drupal.org/node/424758, but the patch does not apply:
What's the best course of action? Start over, or do a reroll from an earlier branch than 00339b3d?
Comment #12
alimac CreditAttribution: alimac commented10: drupal-rename_views_properties_inoperator-2183421-10.patch queued for re-testing.
Comment #14
evilehk CreditAttribution: evilehk commentedI pulled the latest from 8.x, then switched to a commit that was four months, one day old. Sadly, it was the commit where chx was removed from maintainers.txt :(. In any event, the patch applied cleanly. A rebase with 8.x yielded four merge conflicts which I manually resolved. Then I followed the instructions that alimac linked to in comment #11. After running 'php core/scripts/switch-psr4.sh' one file was removed. I did a commit and a diff against 8.x. The rerolled patch from #10 is attached.
Comment #15
evilehk CreditAttribution: evilehk commentedComment #16
alimac CreditAttribution: alimac commentedThanks, @evilehk. I did the same thing (up to the merge conflicts). @Crisz indicated they were still working on it, so I wonder if some additional work will need to be done even if this passes the testbot tests.
Comment #17
schnippy CreditAttribution: schnippy commentedWe tried to apply the patch in 14 but there were a number of changes to the code base since it had been rolled and the patch failed to apply. So we recreated it, removing the references to the deleted file /core/modules/system/tests/Drupal/system/Tests/Menu/SystemLocalTasksTest.php and then fixed the updated InOperator file. This updated patch contains all the changes and successfully applied in our tests.
Comment #18
kgoel CreditAttribution: kgoel commentedfixing the status so test bot can test the last submitted patch
Comment #21
alimac CreditAttribution: alimac commentedPatch in #17 does not apply. Will work on a re-roll.
Comment #22
alimac CreditAttribution: alimac commentedHere's the re-roll -- more work might be required.
Comment #24
alimac CreditAttribution: alimac commentedLooks like a couple of references to `$form_state` in `core/modules/views/src/Plugin/views/filter/BooleanOperator.php` - fixing.
Comment #25
alimac CreditAttribution: alimac commentedRe-rolled for latest HEAD.
Comment #26
alimac CreditAttribution: alimac commentedComment #27
alimac CreditAttribution: alimac commentedI think this should be changed to valueTitle
And this line too.
Comment #28
alimac CreditAttribution: alimac commentedI think
var $valueTitle = NULL;
should be declared inclass InOperator
the way that$valueOptions
is declared, but I'm not sure. This might be out of scope for this issue.Comment #29
YesCT CreditAttribution: YesCT commentedjust the properties are lowerCamelCase. The function vars use _.
----
gave it a quick look and that is all that stuck out for me.
Comment #30
dawehnerTrue,
it should be protected $valueOptions, if possible.
We don't use camelCase for local methods.
Comment #31
alimac CreditAttribution: alimac commentedRestored underscores for local variables per #29, and added
protected
for$valueOptions
and$valueTitle
, per #30.Comment #32
YesCT CreditAttribution: YesCT commentedwe can add a short one line description. and get rid of the whitespace at the end of the line.
why NULL and not '' ? I'm not sure how to tell.
Comment #33
alimac CreditAttribution: alimac commentedI removed the extra space mentioned in #32. Also removed 2 extra spaces that had been introduced at some point.
Since there wasn't a default value set before, changed
protected $valueTitle = NULL;
to just a declaration:protected $valueTitle;
Comment #34
alimac CreditAttribution: alimac commentedThis variable should not have been changed from underscore to camelCase.
Comment #35
alimac CreditAttribution: alimac commentedRestored variable name, and also removed an extra newline that crept in.
Comment #36
alimac CreditAttribution: alimac commentedWhile working on this issue I noticed a typo in variable name in HandlerFilterPermissionTest.php (
$permisssion_by_module
) so I opened #2346215: In HandlerFilterPermissionTest fix typo in variable name, which is not related to this issue.Comment #37
dawehnerSame thing :(
Comment #38
alimac CreditAttribution: alimac commentedRestored local variable
$value_options
.Comment #39
alimac CreditAttribution: alimac commentedComment #40
mon_franco CreditAttribution: mon_franco commentedComment #41
mon_franco CreditAttribution: mon_franco commentedI have reviwed this issue applying the patch with --color-words and looking at every change on it. The variables valueTitle and valueOptions are always correct.
In some of this files I have detected little coding standars errors but involving the patch.
Comment #42
alexpottCommitted 3bb1ac5 and pushed to 8.0.x. Thanks!