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

fix location sanitizing with parser 'future' #301

Merged
merged 1 commit into from
May 27, 2014

Conversation

yath
Copy link
Contributor

@yath yath commented Apr 19, 2014

With parser 'future', a '' counts as one backslash whereas with
parser 'current' it's two. Therefore with 'future' only one gets passed
to regsubst which in turn aborts with

Error: regsubst(): Bad regular expression `'

Probably the 'current' behaviour is a bug, as
http://docs.puppetlabs.com/puppet/latest/reference/lang_datatypes.html#single-quoted-strings
states:

When a literal double backslash is intended, a quadruple backslash must
be used.

Yet, fixing either parser will break code so test here if '' is two
characters or one. Clumsy, but works.

@jfryman
Copy link
Contributor

jfryman commented Apr 22, 2014

I'm not sure I want to commit this. It's annoying, for sure, but I'd rather track a fix upstream since future-parser is still experimental.

I'd love to have conversation about this from the community.

@yath
Copy link
Contributor Author

yath commented May 3, 2014

Hmm. I was just about to report this to puppetlabs and came across this one: https://tickets.puppetlabs.com/browse/PUP-1814

I don't think there's anything left to fix in upstream :-)

With parser 'future', a '\\' counts as one backslash whereas with
parser 'current' it's two. Therefore with 'future' only one gets passed
to regsubst which in turn aborts with

 Error: regsubst(): Bad regular expression `\'

The 'future' behaviour is indeed the correct one but 'current' won't be
fixed either. Therefore, use "\\\\" which produces two backslashes with
both parser (ref: https://tickets.puppetlabs.com/browse/PUP-1814).
@yath
Copy link
Contributor Author

yath commented May 5, 2014

I've updated the PR with a less ugly fix.

@khaefeli
Copy link

same error here. I can confirm the bugfix from @yath is working.

I tried to find out, how the guys from https://puphpet.com/ fixed the error. But dont figured it out.
(they also using this nginx module over nginx::resource::location)

@abraham1901
Copy link
Contributor

👍
Fix - work.
@jfryman please merge.

@jfryman
Copy link
Contributor

jfryman commented May 27, 2014

🆒. Thanks for comments. The streamlined code looks much more palatable.

Thanks for the code! ❤️

jfryman pushed a commit that referenced this pull request May 27, 2014
fix location sanitizing with parser 'future'
@jfryman jfryman merged commit 5933b36 into voxpupuli:master May 27, 2014
@abraham1901
Copy link
Contributor

👍

cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Oct 23, 2017
fix location sanitizing with parser 'future'
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.

4 participants