Files: 
CommentFileSizeAuthor
#19 2002934-19.patch6.5 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,030 pass(es).
[ View ]
#19 interdiff-2002934-19.txt4.79 KBdamiankloip
#15 2002934-15.patch6.45 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,252 pass(es).
[ View ]
#15 interdiff-2002934-15.txt3.63 KBdamiankloip
#12 rename_views_method_validate_argument-2002934-2.patch3.83 KBdrupee
FAILED: [[SimpleTest]]: [MySQL] 57,388 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#11 rename_views_method_validate_argument-2002934-1.patch4.23 KBdrupee
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.
[ View ]
#10 rename_views_method_validate_argument-2002934.patch4.23 KBdrupee
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch rename_views_method_validate_argument-2002934.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 drupal-rename_views_method_validate_argument-7449790-8.patch7.55 KBheddn
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.
[ View ]
#4 drupal-rename_views_method_validate_argument-7449790-4.patch4.25 KBkevin.reiss
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.
[ View ]
#2 drupal-rename_views_method_validate_argument-7449790-2.patch4.92 KBx.algorithm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.
[ View ]

Comments

x.algorithm’s picture

Assigned:Unassigned» x.algorithm

I'll take this on!

x.algorithm’s picture

StatusFileSize
new4.92 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.
[ View ]

3 Files were changed.

Each function renamed as requested.
I believe it should solve the problem.
Please review

x.algorithm’s picture

Status:Active» Needs review
kevin.reiss’s picture

StatusFileSize
new4.25 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.
[ View ]

Added public keyword to methods, and removed an arguement name change from simpletest file,

           default_argument_user: '0'
           default_argument_fixed: ''
           default_argument_php: ''
-          validate_argument_node_type:
+          validateArgument_node_type:
             page: '0'
             story: '0'
kevin.reiss’s picture

There appears to be two "validateArgument()" methods defined in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.

kevin.reiss’s picture

Currently the following two methods are defined in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.

Starting on line 941:

public function validateArgument($arg) {
    // By using % in URLs, arguments could be validated twice; this eases
    // that pain.
    if (isset($this->argument_validated)) {
      return $this->argument_validated;
    }

    if ($this->is_exception($arg)) {
      return $this->argument_validated = TRUE;
    }

    $plugin = $this->get_plugin('argument_validator');
    return $this->argument_validated = $plugin->validate_argument($arg);
  }

  /**
   * Called by the menu system to validate an argument.
   *
   * This checks to see if this is a 'soft fail', which means that if the
   * argument fails to validate, but there is an action to take anyway,
   * then validation cannot actually fail.
   */
  function validate_argument($arg) {
    $validate_info = $this->default_actions($this->options['validate']['fail']);
    if (empty($validate_info['hard fail'])) {
      return TRUE;
    }

    $rc = $this->validateArgument($arg);

    // If the validator has changed the validate fail condition to a
    // soft fail, deal with that:
    $validate_info = $this->default_actions($this->options['validate']['fail']);
    if (empty($validate_info['hard fail'])) {
      return TRUE;
    }

    return $rc;
  }

Before the second one (validate_argument) is camel cased it should be renamed so it won't conflict with the existing validateArgument method.

kevin.reiss’s picture

Status:Needs review» Needs work
heddn’s picture

Status:Needs work» Needs review
StatusFileSize
new7.55 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.
[ View ]

Let's try another pass at this. This should catch (hopefully) all the re-names.

Status:Needs review» Needs work

The last submitted patch, drupal-rename_views_method_validate_argument-7449790-8.patch, failed testing.

drupee’s picture

StatusFileSize
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch rename_views_method_validate_argument-2002934.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Replaced function name and removed syntax error.

drupee’s picture

StatusFileSize
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php.
[ View ]

Please ignore previous patch.

drupee’s picture

StatusFileSize
new3.83 KB
FAILED: [[SimpleTest]]: [MySQL] 57,388 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

I just noticed validateArgument is already present in ArgumentPluginBase.php for some other reason (to double check and get sure of argument).
According to me it's matter of choice which one to be renamed.
But I guess the one which is made for menu systems should be like it is. So I left it, rest are renamed, tried test on local, passed.

drupee’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, rename_views_method_validate_argument-2002934-2.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new3.63 KB
new6.45 KB
PASSED: [[SimpleTest]]: [MySQL] 57,252 pass(es).
[ View ]

I think a couple of conversions were missed outside of the views module, also, I think making the menu method validateMenuArgument is a good idea. We need to camelCase things either way. Let's try this.

heddn’s picture

Status:Needs review» Needs work

This is missing access modifiers of public on several functions. And I think if you change the name to validateMenuArgument, then it should be changed everywhere. For this mass conversion effort, why don't we leave it as validateArgument and we can come back later and decide on a better name.

drupee’s picture

Agreed, Thanks for detailed explanation.

drupee’s picture

Status:Needs work» Needs review

#15: 2002934-15.patch queued for re-testing.

damiankloip’s picture

Assigned:x.algorithm» Unassigned
StatusFileSize
new4.79 KB
new6.5 KB
PASSED: [[SimpleTest]]: [MySQL] 56,030 pass(es).
[ View ]

@heddn, no I don't think so. I think you are getting things confused slightly - The patch in #15 is correct (hence the tests passed); not all the other instances need to be changed. This is not a case of renaming all validateArgument calls.

The current validate_argument() on *ArgumentPluginBase* is what I have changed to validateMenuArgument. There is already a validateArgument on this class. We also have validateArgument on argument validator plugins.

If we don't change this then we still get left with a validate_argument method, and that's not what we want.

Status:Needs review» Needs work
Issue tags:-Novice, -VDC

The last submitted patch, 2002934-19.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
Issue tags:+Novice, +VDC

#19: 2002934-19.patch queued for re-testing.

heddn’s picture

Status:Needs review» Needs work

@damiankloip, that makes more sense now. I went through Git and found where we used to have two functions with similar names.
validate_arg()
&
validate_argument
The first was renamed in #1763678: cleanup methods of PluginBase. This one is picking up the last of these two similarly named functions.

Shouldn't the implementations of ArgumentPluginBase also use the name validateMenuArgument? That's my guess why tests are failing.

damiankloip’s picture

Status:Needs work» Needs review

No, The test was a random failure - see the results on qa.drupal.org.

EDIT: See #19 now....

On ArgumentPluginBase we already have validateArgument. The CURRENT validate_argument method is the only one used. That is the one being renamed.

Please stop moving the status to 'Needs work' when you are making assumptions and asking questions.

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed c06bf84 and pushed to 8.x. Thanks!

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