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

Drainpipe not reading correctly the URLs for visual diffs #625

Closed
beto-aveiga opened this issue Jul 30, 2024 · 5 comments · Fixed by #681
Closed

Drainpipe not reading correctly the URLs for visual diffs #625

beto-aveiga opened this issue Jul 30, 2024 · 5 comments · Fixed by #681
Assignees
Labels
bug Something isn't working client affected

Comments

@beto-aveiga
Copy link
Collaborator

When an override file is used for Tugboat config, it may produce URLs for visual diffs that break Tugboat builds.
If there is a way to build these URLs it should be documented. I tried double quotes and scaping them, but it didn't work.

image

On Tugboat

image

Tags

composer install, .tugboat/config.drainpipe-override.yml, tugboat, visual diffs, visualdiffs

@beto-aveiga beto-aveiga changed the title Drainpipe not reading correctly Drainpipe not reading correctly the URLs for visual diffs Jul 30, 2024
@mrdavidburns mrdavidburns added bug Something isn't working client affected labels Aug 29, 2024
@mrdavidburns
Copy link
Member

This would be a blocker now that our static tests are checking for uncommitted changes.

@beto-aveiga
Copy link
Collaborator Author

This would be a blocker now that our static tests are checking for uncommitted changes.
I'm not clear about this @mrdavidburns

I just think that overriding URLs with parameters for VIsual Diffs doesn't work as expected.

@mrdavidburns
Copy link
Member

@mrdavidburns
Copy link
Member

mrdavidburns commented Aug 29, 2024

@beto-aveiga Looks like this is the section of code where query parameters are being encoded. Thoughts on how we can fix that?

https://github.com/Lullabot/drainpipe/pull/441/files#diff-fccaa14362cd8508cb094a15287d2e442fc57d94205c6b2146087a552815d1fcR346-R356

            // Filter out unsupported config overrides.
            if (!empty($tugboatConfigOverride['php']) && is_array($tugboatConfigOverride['php'])) {
                $tugboatConfigOverride['php'] = array_filter($tugboatConfigOverride['php'], function($key) {
                    return in_array($key, ['aliases', 'urls', 'visualdiff', 'screenshot']);
                }, ARRAY_FILTER_USE_KEY);
                $overrideOutput = [];
                foreach (explode(PHP_EOL, Yaml::dump($tugboatConfigOverride['php'], 2, 2)) as $line) {
                    $overrideOutput[] = str_repeat(' ', 4) . $line;
                }
                $tugboatConfig['overrides']['php'] = rtrim(implode("\n", $overrideOutput));
            }

Or should that be fixed when we insert these values back into the tugboat config file?
https://github.com/Lullabot/drainpipe/pull/441/files#diff-fccaa14362cd8508cb094a15287d2e442fc57d94205c6b2146087a552815d1fcR407-R410

                // Reinstate the override file.
                if (isset($tugboatConfigOverrideFile)) {
                    file_put_contents('./.tugboat/config.drainpipe-override.yml', $tugboatConfigOverrideFile);
                }

Seems YAML::dump() is doing the encoding.

Here's what ChatGPT is suggesting to allow query parameters.
https://chatgpt.com/share/91f4b461-a8e5-4aae-9bda-9da19ff2ddee

@beto-aveiga
Copy link
Collaborator Author

beto-aveiga commented Aug 29, 2024

Thoughts on how we can fix that?

My first idea is to encode them before or encode them after, and see what happens when I run composer install.
However, Xdebugging and seeing the values in real-time would be easier and more effective for discovering new approaches.

Writing a simple test sounds reasonable, too.

@mrdavidburns mrdavidburns self-assigned this Aug 29, 2024
@beto-aveiga beto-aveiga linked a pull request Sep 4, 2024 that will close this issue
mrdavidburns pushed a commit that referenced this issue Sep 4, 2024
* ISSUE-625: output php configurations without scaping them

* ISSUE-625: test url with parameters

* ISSUE-625: add url with parameters

* ISSUE-625: fix visual diff with parameters end result

* ISSUE-625: silly typo :p

* ISSUE-625: not needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working client affected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants