Skip to content
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

Support replacing multiple custom template tags in one output path. #15596

Merged
merged 3 commits into from
May 15, 2019

Conversation

TimothyBJacobs
Copy link
Member

Fixes #15567

Description

Instead of returning after the first template match, continue looping and just update the path variable.

How has this been tested?

I've tested the change in my project and it works as expected.

I really have no idea how to unit test this.

Types of changes

Bug Fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Would you be able to add a unit test to capture what's being fixed?

@TimothyBJacobs
Copy link
Member Author

Cool! I'd like to add a unit test, but I'm not really sure what the path forward is for something like this. I could combine it into the existing test? Or should I create a whole new fixture for multiple template tags?

@aduth
Copy link
Member

aduth commented May 14, 2019

@TimothyBJacobs If it's simple enough to include it in the existing test, that would be fine, as long as it provides sufficient coverage to both the base functionality and this specific fixed behavior (I think it should be).

@aduth
Copy link
Member

aduth commented May 14, 2019

I added a simple test case which verifies the fix (fails in master) in b456b3b.

@aduth aduth merged commit fc76329 into WordPress:master May 15, 2019
@aduth
Copy link
Member

aduth commented May 15, 2019

Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CustomTemplatedPathPlugin does not support replacing multiple template tags.
3 participants