-
Notifications
You must be signed in to change notification settings - Fork 14
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
Issue #625: Tugboat urls support query parameters. #673
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused by this PR.
At the least, I think doing a broad htmldecode on the YAML itself could lead to other bugs. For example, what happens if you actually want an HTML entity in some other text? What if you are inlining HTML?
Reading the code, it's as if Yaml::dump()
is encoding HTML entities. But, I did a quick test at https://github.com/deviantintegral/yaml-test and it doesn't:
/opt/homebrew/bin/php /Users/andrew/workspace/yaml-test/test.php
urls:
- '/path?query=value'
Process finished with exit code 0
Do you think you can narrow this down a bit into something more targeted that tells us exactly what function is doing the HTML encoding? I like @beto-aveiga 's suggestion of xdebug!
// Prepare YAML data | ||
$yamlData = []; | ||
foreach ($tugboatConfigOverride['php'] as $key => $value) { | ||
$yamlData[$key] = $value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong - as in, I suppose it works, but don't you just want something like:
$yamlData = $tugboatConfigOverride['php'];
$tugboatConfigOverride['php'] = array_filter($tugboatConfigOverride['php'], | ||
function($key) { | ||
return in_array($key, | ||
['aliases', 'urls', 'visualdiff', 'screenshot']); | ||
}, | ||
ARRAY_FILTER_USE_KEY); | ||
$tugboatConfigOverride['php'] = array_filter($tugboatConfigOverride['php'], function($key) { | ||
return in_array($key, ['aliases', 'urls', 'visualdiff', 'screenshot']); | ||
}, ARRAY_FILTER_USE_KEY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirming, this appears to only be formatting changes?
Closing in favor of #681 |
No description provided.