Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Assigned: Unassigned »
Status: Active » Needs review
FileSize
9.79 KB

Status: Needs review » Needs work

The last submitted patch, 1: drupal-rename_wiews_properties-2183421-1.patch, failed testing.

The last submitted patch, 1: drupal-rename_wiews_properties-2183421-1.patch, failed testing.

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
5.9 KB
5.98 KB

New patch attached.

Status: Needs review » Needs work

The last submitted patch, 4: drupal-rename_views_properties_inoperator-2183421-4.patch, failed testing.

sidharthap’s picture

Assigned: » Unassigned
Issue tags: -Novice, -VDC +drupalcampmumbai sprint

Updating issue tag. Will have a look at Drupal Camp Mumbai sprint on this weekend.

Crisz’s picture

Crisz’s picture

Status: Needs work » Needs review

The last submitted patch, 7: drupal-rename_views_properties_inoperator-2183421-7.patch, failed testing.

Crisz’s picture

Still working on it.

alimac’s picture

The 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:

error: patch failed: core/modules/comment/lib/Drupal/comment/Plugin/views/filter/NodeComment.php:19
error: core/modules/comment/lib/Drupal/comment/Plugin/views/filter/NodeComment.php: patch does not apply
error: patch failed: core/modules/field/lib/Drupal/field/Plugin/views/filter/FieldList.php:20
error: core/modules/field/lib/Drupal/field/Plugin/views/filter/FieldList.php: patch does not apply
error: patch failed: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.php:358
error: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.php: patch does not apply
error: patch failed: core/modules/views/lib/Drupal/views/Plugin/views/filter/InOperator.php:342
error: core/modules/views/lib/Drupal/views/Plugin/views/filter/InOperator.php: patch does not apply

What's the best course of action? Start over, or do a reroll from an earlier branch than 00339b3d?

alimac’s picture

Status: Needs review » Needs work

The last submitted patch, 10: drupal-rename_views_properties_inoperator-2183421-10.patch, failed testing.

evilehk’s picture

I 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.

evilehk’s picture

Status: Needs work » Needs review
alimac’s picture

Thanks, @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.

schnippy’s picture

Status: Needs review » Patch (to be ported)
Issue tags: -drupalcampmumbai sprint +sprint
FileSize
22.13 KB

We 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.

kgoel’s picture

Status: Patch (to be ported) » Needs review

fixing the status so test bot can test the last submitted patch

Status: Needs review » Needs work

The last submitted patch, 17: in_inoperator_rename-2183421-17.patch, failed testing.

alimac’s picture

Issue tags: +Needs reroll

Patch in #17 does not apply. Will work on a re-roll.

alimac’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
22.71 KB

Here's the re-roll -- more work might be required.

Status: Needs review » Needs work

The last submitted patch, 22: in_inoperator_rename-2183421-22.patch, failed testing.

alimac’s picture

Status: Needs work » Needs review
FileSize
24.4 KB
2.32 KB

Looks like a couple of references to `$form_state` in `core/modules/views/src/Plugin/views/filter/BooleanOperator.php` - fixing.

alimac’s picture

Issue tags: -sprint +sprint Amsterdam2014
FileSize
23.58 KB

Re-rolled for latest HEAD.

alimac’s picture

Issue tags: -sprint Amsterdam2014 +sprint, +Amsterdam2014
alimac’s picture

  1. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
    @@ -48,7 +48,7 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
     
    

    I think this should be changed to valueTitle

  2. +++ b/core/modules/views/src/Plugin/views/filter/LanguageFilter.php
    @@ -23,9 +23,9 @@ class LanguageFilter extends InOperator {
           $this->value_title = $this->t('Language');
    

    And this line too.

alimac’s picture

I think var $valueTitle = NULL; should be declared in class InOperator the way that $valueOptions is declared, but I'm not sure. This might be out of scope for this issue.

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
@@ -80,8 +80,8 @@ public function defaultExposeOptions() {
-  public function buildExposeForm(&$form, FormStateInterface $form_state) {
-    parent::buildExposeForm($form, $form_state);
+  public function buildExposeForm(&$form, FormStateInterface $formState) {
+    parent::buildExposeForm($form, $formState);

just the properties are lowerCamelCase. The function vars use _.

----

gave it a quick look and that is all that stuck out for me.

dawehner’s picture

True,

  1. +++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
    @@ -32,7 +32,7 @@ class InOperator extends FilterPluginBase {
    -  var $value_options = NULL;
    +  var $valueOptions = NULL;
    

    it should be protected $valueOptions, if possible.

  2. +++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
    @@ -199,21 +199,21 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
    -          $default_value = array_shift($keys);
    +          $defaultValue = array_shift($keys);
    

    We don't use camelCase for local methods.

alimac’s picture

Status: Needs work » Needs review
FileSize
19.08 KB
7.12 KB

Restored underscores for local variables per #29, and added protected for $valueOptions and $valueTitle, per #30.

YesCT’s picture

+++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
@@ -32,7 +32,12 @@ class InOperator extends FilterPluginBase {
+  /**
+   * @var string
+   */
+  protected $valueTitle = NULL; ¶

we 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.

alimac’s picture

I 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;

alimac’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
@@ -428,11 +435,11 @@ public function validate() {
-      $flat_options = form_options_flatten($this->value_options, TRUE);
+      $flatOptions = form_options_flatten($this->valueOptions, TRUE);

This variable should not have been changed from underscore to camelCase.

alimac’s picture

Status: Needs work » Needs review
FileSize
18.41 KB
1.51 KB

Restored variable name, and also removed an extra newline that crept in.

alimac’s picture

While 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.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/src/Tests/Views/HandlerFilterPermissionTest.php
@@ -78,7 +78,7 @@ public function testFilterPermission() {
-    $value_options = $view->filter['permission']->getValueOptions();
+    $valueOptions = $view->filter['permission']->getValueOptions();

@@ -90,7 +90,7 @@ public function testFilterPermission() {
-      $this->assertEqual($expected, $value_options[$title], 'Ensure the all permissions are available');
+      $this->assertEqual($expected, $valueOptions[$title], 'Ensure the all permissions are available');

Same thing :(

alimac’s picture

Restored local variable $value_options.

alimac’s picture

Status: Needs work » Needs review
mon_franco’s picture

Assigned: Unassigned » mon_franco
mon_franco’s picture

Assigned: mon_franco » Unassigned
Status: Needs review » Reviewed & tested by the community

I 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3bb1ac5 and pushed to 8.0.x. Thanks!

  • alexpott committed 3bb1ac5 on 8.0.x
    Issue #2183421 by alimac, Crisz, InternetDevels, schnippy, evilehk,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.