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

Don't crash on windows #2

Merged
merged 3 commits into from
Sep 22, 2020
Merged

Conversation

demeritcowboy
Copy link
Contributor

Windows doesn't like the multiline command.

Windows doesn't like the multiline command.
@seamuslee001
Copy link
Contributor

I tested this and it works for me @totten

@totten
Copy link
Member

totten commented Sep 22, 2020

@demeritcowboy @seamuslee001 Ah, interesting problem.

So at first glance, I thought this was OK - but then it struck me: "What data in that command involves newline characters?" Most of the args shouldn't. It's probably the $event->getTask()->definition, because that's an open-ended thing. I guess that raised problems on account of var_export()s formatting. But the $definition is user-supplied data, and there could be other wonky stuff in there.

Are you guys down with 90990bf - i.e. passing the definition through a base64 filter? On an example build (per ScssPhpMethodTest.php) on Linux, it runs the subcommand as follows:

> shell-runner: @php -r 'require_once '\''/home/totten/src/sandbox/vendor/autoload.php'\'';
 \ScssExample\ScssExample::make(json_decode(base64_decode('\''eyJhY3Rpdm
UiOnRydWUsInRpdGxlIjoiPGNvbW1lbnQ+dGVzdFwvZ25vY2NoaTxcL2NvbW1lbnQ+Ojxjb2
1tZW50PjE8XC9jb21tZW50PiIsIndhdGNoLWZpbGVzIjpudWxsLCJwaHAtbWV0aG9kIjoiXF
xTY3NzRXhhbXBsZVxcU2Nzc0V4YW1wbGU6Om1ha2UiLCJzb3VyY2UtZmlsZSI6IlwvaG9tZV
wvdG90dGVuXC9zcmNcL3NhbmRib3hcL3ZlbmRvclwvdGVzdFwvZ25vY2NoaVwvY29tcG9zZX
IuanNvbiJ9'\''), 1));'

(Note: the command actually appears as one-line, but I've edited for readability on github ui).

@demeritcowboy
Copy link
Contributor Author

It's clever - give me a few minutes to try it out.

@demeritcowboy
Copy link
Contributor Author

Runs good. There's just some whitespace that snuck into line 49.

@totten
Copy link
Member

totten commented Sep 22, 2020

Good catch @demeritcowboy. (Apparently my PSR-12 style-checker doesn't complain about that...)

Whitespace fixed. Tests pass again. Merging.

@totten totten merged commit 7d4a89f into civicrm:master Sep 22, 2020
@totten
Copy link
Member

totten commented Sep 22, 2020

Tagged as v0.7.

@demeritcowboy
Copy link
Contributor Author

Thanks guys!

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.

3 participants