Merge HandlerBase::breakPhrase() and HandlerBase::breakPhraseString() and clean up

If we make the handler mandatory i think it would make sense to remove the static from it, as it will always alter the handler object.

My initial thought was to create an empty handler implementation. (a dummy class)

I am working on it.

What we did talk about at the vdc sprint is that:

1. Merge breakPhrase() and breakPhraseString()
2. Just return an array with array('values' => array(), 'operator' => 'and');

So I am renaming the issue.

sudo drush-git si standard --db-url=mysql://d8:d8@localhost/d8 --site-name="D8 Test Site"

This is a first version. But I can't test it locally because of some problems with my virtualbox. Please tell me if this is going into the right direction.

Some comments about why I did the patch in this way.

• I merged breakPhrase and breakPhraseString because they are so similar and moved the int check to an optinal parameter.
• I removed the regular expressions because the explode of the string can be easily be exploded if we ignore the " " space problem. The Help text for the filter clearly states that it should be "+" or "," and nothing about "+ ". And dawehner mentioned that the " " probably would be URL escepd to "+" anyways and won't matter.
• The check for is_int() uses a straight foreach like the old function did additionally. I believe this is much easier to read this way than checking and exploding with a regex.
• There where a lot calls like $this->breakPhrase() But it is a static function and it should use "::". I fixed that but have no idea why this did work at all. I used a debugger and the code with $this->breakPhrase() did work as intended.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -814,117 +814,47 @@ public static function getTableJoin($table, $base_table) {
+        if (!is_int($value)) {
+          $return['value'] = array();
+          break;

Maybe it make sense to cast the value to a integer, so we don't end up stopping if we have a single string in there.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ManyToOne.phpundefined
@@ -117,12 +117,11 @@ public function query($group_by = FALSE) {
+      $force_int = empty($this->definition['numeric']) ? FALSE : TRUE;

empty is already a boolean so we just could use $force_int = !empty($this->definition['numeric']);

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ManyToOne.phpundefined
@@ -117,12 +117,11 @@ public function query($group_by = FALSE) {
+      $breakPhrase = HandlerBase::breakPhrase($this->argument, $force_int);

We should certainly use $break_phrase instead of $breakPhrase or even list($value, $operator);

Updated version with your suggestions. And I found more uses of breakPhrase() and fixed them. Please review.

#8: drupal-1792836-breakPhrase-v02.patch queued for re-testing.

Let's try and revive this issue :)

I have made quite a few changes here:

• Set a default array in breakPhrase, so we are always getting an array of atleast array('operator' => NULL, 'value' => NULL). If the string was invalid we would have an operator but no values. If we can get rid of the array(-1) I think that would be preferable. Even if we have to set a 'broken' array element or something.
• Got back regular expressions and modified them to work with two break characters E.g. 'damian++kloip' => array('damian', '', 'kloip')
• Changed list() usage to extract() in tests, as list doesn't work with associative arrays.
• Fixed tests and removed assertEqualValue method.

So, I'm not sure what behaviour we want when $enforce_int is true, at the moment it gets casted, so a string will become 0 instead. The tests had NULL being returned instead of any values before. Ideas?

This one might pass.

I'm not sure about using extract().

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_validator/Node.phpundefined
@@ -109,17 +109,16 @@ function validate_argument($argument) {
+        list($value, $operator) = HandlerBase::breakPhrase($this->argument, TRUE);
...
+        $test = drupal_map_assoc($value);
...
+        $nodes = node_load_multiple($value);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/IndexTidDepth.phpundefined
@@ -102,21 +102,14 @@ public function query($group_by = FALSE) {
+      list($value) = HandlerBase::breakPhrase($this->argument);

Wouldn't it be easier if we call $value $nids/$tids, so it's clear what actually is going on here?

Another comment: Couldn't we use get_class($this->argument)::breakPhrase here?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -826,117 +826,45 @@ public function getEntityType() {
+  public static function breakPhrase($str, $force_int = FALSE) {

Any using not to rename it to "string"?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -826,117 +826,45 @@ public function getEntityType() {
+      foreach ($return['value'] as $key => &$value) {
+        $value = (int) $value;
+      }

What about array_map('intval', $return['value'])?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ManyToOne.phpundefined
@@ -122,12 +122,11 @@ public function query($group_by = FALSE) {
+      extract(static::breakPhrase($this->argument, $force_int));

@@ -143,7 +142,10 @@ function title() {
+      extract(static::breakPhrase($this->argument, $force_int));

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/Numeric.phpundefined
@@ -69,7 +69,9 @@ function title() {
+      extract(static::breakPhrase($this->argument, TRUE));

@@ -100,7 +102,9 @@ public function query($group_by = FALSE) {
+      extract(static::breakPhrase($this->argument, TRUE));

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/String.phpundefined
@@ -191,7 +191,9 @@ public function query($group_by = FALSE) {
+      extract(static::breakPhrase($this->argument));

@@ -262,7 +264,9 @@ function title() {
+      extract(static::breakPhrase($this->argument));

For two variables I see no reason in using extract? I think list() is at least way easier to read.

Ok, made most of those changes and switched back to list, I guess it's slightly less evil...

Not sure about the first comment. $this->argument should be a string now?

Oh, hang on. That part in Node.php argument validator is wrong anyway. It's passing the argument into the breakPhrase, this should be just passing in $argument and not $this->argument I think. We should try and change this to your suggestion of using get_class too.

The test results are strage.

Value array ( 0 => 0, 1 => 29, 2 => 13, ) is equal to value array ( 0 => 0, 1 => 29, 2 => 13, ).

This is the test in line 142 with "cast to int". So why is it still "0,1" and not an int? PHP Type Juggling?
The tests should get some messages so we can be sure what the result should be.

As you still do " $value = preg_split('/[+ ]/', $str);" I believe the help text should change to reflect that spaces can be used as "or". The string "If selected, users can enter multiple values in the form of 1+2+3 (for OR) or 1,2,3 (for AND)." is used in three different places. Is there any chance that this can be put and used from the HandlerBase object?

#16: 1792836-16.patch queued for re-testing.

Add tag

re-rolled the patch

made some changes to the HandlerBase class and test. needs review.

Do you have an interdiff of the changes?

let me get the patch to pass the SimpleTests. I will then upload an interdiff.

I am having difficulty getting this patch to pass the tests. The problem seems to be with the regex to detect whether the operation is AND or OR. It fails on parameters like 'word1', 'wõrd1' etc.

Thanks for your work picking this back up!

Here is a quick bit of work on here. Converted the usage of breakString() from using list(), as this method now returns a stdClass object.

Also, I don't think we need to try and hardcode specific characters like this if we are using the u flag in the regex /^([\X{00C0}-\x{01FF}\w\-\d]+[+ ]+)+[\X{00C0}-\X{01FF}\w\-\d]+$/u. I think just unicode mode will suffice?

We don't want to filter out 0 values from out breaking of strings.

31: 1792836-31.patch queued for re-testing.

I do love

+++ b/core/modules/views/src/Plugin/views/HandlerBase.php
@@ -783,117 +783,41 @@ public function getEntityType() {
+    return (object) array('value' => $value, 'operator' => $operator);

meh, if we would just have proper support in the language.

1. +++ b/core/modules/views/src/Plugin/views/HandlerBase.php
   @@ -783,117 +783,41 @@ public function getEntityType() {
   -  /**
       * Breaks x,y,z and x+y+z into an array. Works for strings.
       *
       * @param string $str
   

   The phrase "Works for strings" is now unneeded.

2. +++ b/core/modules/views/src/Plugin/views/HandlerBase.php
   @@ -783,117 +783,41 @@ public function getEntityType() {
   +   * @param bool $force_int
   +   *   Enforce a numeric check.
   

   You are not really forcing a numeric check - you are forcing all values to have intval() run on them. Which is interesting - just do php -r "print intval('0452rtrtw');"

3. +++ b/core/modules/views/src/Plugin/views/HandlerBase.php
   @@ -783,117 +783,41 @@ public function getEntityType() {
   +   * @return object $handler
   

   Nope it does not return a handler anymore.

4. +++ b/core/modules/views/src/Plugin/views/argument/ManyToOne.php
   @@ -119,12 +119,11 @@ public function query($group_by = FALSE) {
   +      $break = static::breakString($this->argument, $force_int);
   +      $this->value = $break->value;
   +      $this->operator = $break->operator;
   
   @@ -140,7 +139,10 @@ function title() {
   +      $break = static::breakString($this->argument, $force_int);
   +      $this->value = $break->value;
   +      $this->operator = $break->operator;
   
   +++ b/core/modules/views/src/Plugin/views/argument/Numeric.php
   @@ -65,7 +65,9 @@ function title() {
   +      $break = static::breakString($this->argument, TRUE);
   +      $this->value = $break->value;
   +      $this->operator = $break->operator;
   
   @@ -96,7 +98,9 @@ public function query($group_by = FALSE) {
   +      $break = static::breakString($this->argument, TRUE);
   +      $this->value = $break->value;
   +      $this->operator = $break->operator;
   
   +++ b/core/modules/views/src/Plugin/views/argument/String.php
   @@ -188,7 +188,9 @@ public function query($group_by = FALSE) {
   +      $break = static::breakString($this->argument);
   +      $this->value = $break->value;
   +      $this->operator = $break->operator;
   
   @@ -262,7 +264,9 @@ function title() {
   +      $break = static::breakString($this->argument);
   +      $this->value = $break->value;
   +      $this->operator = $break->operator;
   

   Seems a shame not to encapsulate this functionality on a protected method on HandlerBase

5. +++ b/core/modules/views/src/Tests/Handler/HandlerTest.php
   @@ -232,8 +229,12 @@ public function testHandlerWeights() {
   +  protected function assertEqualValue($expected, $handler, $message = '', $group = 'Other') {
   +    return $this->assert($expected == $handler->value,
   +        $message ? $message : t('Comparing @first and @second', array(
   +            '@first' => implode(',', $expected),
   +            '@second' => implode(',', $handler->value)
   +        )), $group);
      }
   

   Not proper formatting.

How about this? I have added a method to arguments, as we really only unpack things like that there. Better names for that method welcome!

+++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
@@ -1150,6 +1150,18 @@ public static function encodeValidatorId($id) {
+   * @param bool $force_int
+   *   Enforce that values should be numeric.
+   */
+  protected function unpackArgumentValue() {

+++ b/core/modules/views/src/Plugin/views/argument/ManyToOne.php
@@ -121,10 +121,7 @@ public function query($group_by = FALSE) {
+      $this->unpackArgumentValue($force_int);

@@ -141,9 +138,7 @@ function title() {
+      $this->unpackArgumentValue($force_int);

Missing param

Which param?

Duh. Yes I see the issue :) rerolling...

randomName > randomMachineName

+++ b/core/modules/views/src/Plugin/views/HandlerBase.php
@@ -772,117 +772,42 @@ public function getEntityType() {
+  public static function breakString($str, $force_int = FALSE) {

Yeah we could indeed think about using that functionality in other places in core. One other example could be RoleAccessCheck and so on and forth.

Committed 0946b70 and pushed to 8.0.x. Thanks!