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

ENH Allow multiple backtick variables in a single value #10312

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented May 11, 2022

#10305

Required for the config update in silverstripe/cwp-core#107

@maxime-rainville
Copy link
Contributor

Can you clarify what's the use case this enables that wasn't possible before ... if only for posterity.

This also looks like something that should be documented in the changelog and/or doc?

@emteknetnz
Copy link
Member Author

emteknetnz commented May 11, 2022

Required so that we can apply this config in the cwp-core PR

- proxy: '`SS_OUTBOUND_PROXY`:`SS_OUTBOUND_PROXY_PORT`'

I've made a small update to the docs

@emteknetnz emteknetnz marked this pull request as ready for review May 11, 2022 04:23
@michalkleiner
Copy link
Contributor

I'll play the devil's advocate here this time...

Should be targetting 4 if it's an enhancement?
If we need it to fix 4.11 release (or CWP 2.11 depending on this in 4.11) it should be called a bugfix for a bug where we would say we expected multiple variables to be evaluated and that wasn't working.

@emteknetnz
Copy link
Member Author

emteknetnz commented May 11, 2022

Yes normally I'd do 4, it's just there hasn't been stable tag on 4.11 yet, only 4.11.0-beta1. So it's correct to target 4.11 here so it will be released as 4.11.0

This is ultimately to fix an issue that was spotted during beta regression testing

docs/en/00_Getting_Started/03_Environment_Management.md Outdated Show resolved Hide resolved
src/Core/Injector/Injector.php Outdated Show resolved Hide resolved
tests/php/Core/Injector/InjectorTest.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/4.11/injector branch 3 times, most recently from 1a84ca6 to cf04bef Compare May 12, 2022 01:25
@GuySartorelli
Copy link
Member

GuySartorelli commented May 12, 2022

What's the expected behaviour if someone has config that looks like '`something`somethingelse`'?
I'm pretty sure the current regex will substitute the first "something" but not the "somethingelse" which is fine, just want to make sure what is happening is what is expected.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

The actual functionality looks good. Just some additional test cases to catch edge cases.

tests/php/Core/Injector/InjectorTest.php Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks good. Merge on green.
Once merged, kick off the tests again on silverstripe/cwp-core#107

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.

None yet

4 participants