Problem/Motivation

The argument validator "None" (basic validator) contains $arg as a parameter, but it's not defined anywhere. I'm thinking it was supposed to be made $argument.

The $arg was added in #1788266-28: Access the different views related properties on the right object.
In that issue an error was made when moving code from function validate_argument_basic($arg) to Node::validateArgument($argument). The argument $arg is used in the original function and that codes was moved to the new method validateArgument. But now, in the new method there is no $arg and it was considered unused.
It was removed in #3316276: Fix 'undefined variable' PHPStan L0 errors in a PHPStan issue, which typically do not research the removals.

Steps to reproduce

Proposed resolution

Add a test
Change $arg to $argument

Remaining tasks

Review, specifically the added Test
Commit

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#12 3029364-12.patch3.81 KBquietone
#12 3029364-12-fail.patch3.11 KBquietone
#2 argument-validator-contains-undefined-variable-3029364-2.patch712 bytesandileco

Issue fork drupal-3029364

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

andileco created an issue. See original summary.

andileco’s picture

andileco commented
Status: Active » Needs review
StatusFileSize
new argument-validator-contains-undefined-variable-3029364-2.patch712 bytes

Status: Needs review » Needs work

The last submitted patch, 2: argument-validator-contains-undefined-variable-3029364-2.patch, failed testing. View results

andileco’s picture

andileco commented
Status: Needs work » Needs review

The first time the tests ran, they failed, but the second time they passed.

nickdickinsonwilde’s picture

nickdickinsonwilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
commented
Status: Needs review » Reviewed & tested by the community

I feel like fixing the variable name doesn't need an extra test, but some might disagree.
LGTM

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/views/src/Plugin/views/argument_validator/None.php
@@ -23,7 +23,7 @@ public function validateArgument($argument) {
-    if (!empty($this->argument->definition['numeric']) && !isset($this->argument->options['break_phrase']) && !is_numeric($arg)) {
+    if (!empty($this->argument->definition['numeric']) && !isset($this->argument->options['break_phrase']) && !is_numeric($argument)) {

Well we need test coverage here because it looks like we've never triggered this part of the if. So whatever it is there for is untested.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Needs tests

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
quietone’s picture

quietone commented
Version: 9.3.x-dev » 10.0.x-dev
Assigned: andileco » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
Related issues: +#1788266: Access the different views related properties on the right object
StatusFileSize
new 3029364-12-fail.patch3.11 KB
new 3029364-12.patch3.81 KB

I think this might be my first Views test.

Did some digging and found that $arg was added in 2012 to Views in a rather large patch. #1788266-28: Access the different views related properties on the right object

The last submitted patch, 12: 3029364-12-fail.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 12: 3029364-12.patch, failed testing. View results

quietone’s picture

quietone commented

The fail patch has an unexpected Drupal\KernelTests\Core\Entity\EntityAutocompleteTest::testSelectionSettingsHandling
The success patch has a failure in Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone opened merge request !16183

quietone’s picture

quietone commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs subsystem maintainer review

Converted to an MR and restoring the removed variable that was considered unused.

Tagging for subsystem maintainer review to decide if $arg should be restored or not.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

@quietone I'm not 100% clear on what you are asking. Do you mean restoring the is_numeric check that used to use $arg? If so, then yes we should do that, like the MR is doing now. Or were you referring to something else?

longwave-bot made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: -Needs tests
quietone’s picture

quietone commented
Issue summary: View changes

@lendude. sorry to take so long. I was double checking if the name should be '$arg'. But you have confirmed that the change in the MR is correct.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

@quietone thanks for checking!

I that case I feel this is RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.5.x-dev
Status: Reviewed & tested by the community » Fixed

I've backported this to 12.0.x, 11.x, and 11.5.x as a very low risk bug fix.

Committed and pushed c8ac3d260c9 to main and afd2bcb0f91 to 12.0.x and 4b320955ef4 to 11.x and e80115fc5da to 11.5.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed e80115fc on 11.5.x 
    fix: #3029364 "None" Argument Validator for Views Contains an Undefined...

  • alexpott committed 4b320955 on 11.x 
    fix: #3029364 "None" Argument Validator for Views Contains an Undefined...

  • alexpott committed afd2bcb0 on 12.0.x 
    fix: #3029364 "None" Argument Validator for Views Contains an Undefined...

  • alexpott committed c8ac3d26 on main 
    fix: #3029364 "None" Argument Validator for Views Contains an Undefined...