New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve AutoloadSplitter regexes #4422

Merged
merged 3 commits into from Oct 8, 2017

Conversation

3 participants
@GaryJones
Contributor

GaryJones commented Oct 6, 2017

Two commits - the first is fine, but the second one may be too strict.

  1. Remove leading and trailing .*:
  • With no pinning of the pattern to the start or end of a string, the leading and trailing .* are redundant.
  1. Ensure filename has at least one name before command:
  • This tightens the regex to disallow:

    • /wp-cli/-command/
    • /wp-cli/foo--command/
    • etc.

@GaryJones GaryJones changed the title from Inspection autoload splitter regexes to Improve AutoloadSplitter regexes Oct 6, 2017

@danielbachhuber

Because this includes functional changes, it should include corresponding tests.

@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Oct 6, 2017

Contributor

Because this includes functional changes, it should include corresponding tests.

I'm on it.

Contributor

GaryJones commented Oct 6, 2017

Because this includes functional changes, it should include corresponding tests.

I'm on it.

GaryJones added some commits Oct 6, 2017

Regex: Remove leading and trailing .*
With no pinning of the pattern to the start or end of a string, the leading and trailing `.*` are redundant.
Regex: Ensure filename has at least one name before command
This tightens the regex to disallow:

- `/wp-cli/-command/`
- `/wp-cli/--command/`
etc.
@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Oct 6, 2017

Contributor

First of the rebased commits adds unit tests for existing functionality. You can check out that commit, and run phpunit --group autoloadsplitter and see the tests pass:

phpunit test result

Second commit makes the non-breaking change, and the tests still pass as they were.

The third commit makes the opinionated change that stops /wp-cli/-command/ and similar non-typical paths now be non-conforming (see the change in the test file). Obviously, this is optional, but I'm not sure what benefit there would be to the community to continue allow these non-typical patterns.

Contributor

GaryJones commented Oct 6, 2017

First of the rebased commits adds unit tests for existing functionality. You can check out that commit, and run phpunit --group autoloadsplitter and see the tests pass:

phpunit test result

Second commit makes the non-breaking change, and the tests still pass as they were.

The third commit makes the opinionated change that stops /wp-cli/-command/ and similar non-typical paths now be non-conforming (see the change in the test file). Obviously, this is optional, but I'm not sure what benefit there would be to the community to continue allow these non-typical patterns.

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Oct 8, 2017

Member

Obviously, this is optional, but I'm not sure what benefit there would be to the community to continue allow these non-typical patterns.

Note: this is only used for the WP-CLI bundled commands, not for third-party commands. So, we can do it as strict as we want, as long as we make sure that our own bundled commands still work correctly.

Member

schlessera commented Oct 8, 2017

Obviously, this is optional, but I'm not sure what benefit there would be to the community to continue allow these non-typical patterns.

Note: this is only used for the WP-CLI bundled commands, not for third-party commands. So, we can do it as strict as we want, as long as we make sure that our own bundled commands still work correctly.

@schlessera schlessera added this to the 1.4.0 milestone Oct 8, 2017

Tests were included.

@schlessera schlessera merged commit 10b860f into wp-cli:master Oct 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@GaryJones GaryJones deleted the GaryJones:inspection-autoload-splitter-regexes branch Oct 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment