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

[issue-2176] Added escape/unescape functions #2180

Closed
wants to merge 1 commit into from

Conversation

bsacharski
Copy link
Contributor

@bsacharski bsacharski commented Oct 2, 2020

  • New feature: functions for escaping/unescaping values wrapped in {{}}
  • BC breaks
  • Deprecations
  • Tests added
  • Changelog updated

This commit adds two new functions:

  • escape(string): string
  • unescape(string): string

Once the value has been processed by the escape function, then the values wrapped in {{}} like {{foo}} will not be parsed by the parse function.

The escape function works by replacing the {{ with \\{\\{ and }} with \\}\\}.
The unescape function reverses the process.

The parse function has been updated to run the unescape afterparsing has been completed, to ensure that the commands are being executed normally.

Fixes #2176

This commit adds two new functions:
* `escape(string): string`
* `unescape(string): string`

Once the value has been processed by the `escape` function, then
the values wrapped in `{{}}` like `{{foo}}` will not be parsed by
the `parse` function.

The `escape` function works by replacing the `{{` with `\\{\\}` and
`}}` with `\\}\\}`. The `unescape` function reverses the process.

The `parse` function has been updated to run the `unescape` after
parsing has been completed, to ensure that the commands are being
executed normally.

Fixes deployphp#2176
Copy link
Contributor

@clxmstaab clxmstaab left a comment

Choose a reason for hiding this comment

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

this looks really good, thx

@antonmedv
Copy link
Member

So unescape will be run on each command? Hmmm. What are consecutions? Is there a way it will break something?
Maybe we should think a little about placeholders?

Now there is one special placeholder secret (which will not be printed to stdout nor logs):

run("fjoy -p '%secret%' -fk", ['secret' => getenv('CI_SECRET')]);

What about adding something similar?

run('mysql --silent --raw --skip-column-names --execute %query%', ['vars' => [
    'query' => $query,
]]);

Also, I'm concerned with the name escape. It may confuse people what this is for escaping CLI arguments (deals with quotas, but it does not).

@bsacharski
Copy link
Contributor Author

bsacharski commented Oct 7, 2020

This might be even more complicated than I've previously thought.

The {{foo}} is being replaced everytime the parse method is being called, meaning it'll not only be executed as a part of run but also as a part of get,upload and many other functions/methods.

Maybe the best approach would be simply to do nothing in \Deployer\Configuration\Configuration::parseCallback when the "match" is not defined in configuration values?

And yes, unfortunately, in my approach, the unescape might break some thing if \\{\\{ or \\}\\} are part of a string. It could technically be reduced, by looking for explicitly /\\\\\{\\\\\{\s*([\w\.\/-]+)\s*\\\\\}\\\\\}/, but there's always a risk of matching something defined by user.

@antonmedv
Copy link
Member

So what do you think about placeholders?

@bsacharski
Copy link
Contributor Author

Placeholder will work if the value is being set explicitly, like:

$query = 'select * from {{foo}}';
run('mysql --silent --raw --skip-column-names --execute %query%', ['vars' => [
    'query' => $query,
]]);

if however the code is like:

set('foo', 'select * from {{foo}}');
run('mysql --silent --raw --skip-column-names --execute %query%', ['vars' => [
    'query' => get('foo'),
]]);

then the code will try to parse that {{foo}}.

If we assume that all we want to handle here is run then I guess I could proceed with the different MR ;)

@antonmedv
Copy link
Member

set('foo', 'select * from {{foo}}');

What is the point if this?) From my understanding case is next: a user has some $variable with {{}} in it. What to pass it to run. Placeholder will work in this case.

@bsacharski
Copy link
Contributor Author

bsacharski commented Oct 8, 2020

There is no point ;)

The problem is that we do not know where the value that will be used with run and placeholders will be coming from. If it comes from get then it'll still be parsed. But I guess that's a problem that has not yet happened. It can be dealt with once (if ever) it appears ;)

I'll get to the new MR then ;)

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.

escape method for run command
3 participants