Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Allow binary values in parameters.#25928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[DI] Allow binary values in parameters.#25928
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedJan 25, 2018
Doesn't this apply to 2.7 too? |
| privatefunctiondoExport($value,$resolveEnv =false) | ||
| { | ||
| if (is_string($value) &&false !==strpos($value,"\n")) { | ||
| if (is_string($value) &&preg_match('/[\x00-\x08\x0b-\x1f\x7f-\xff]+/',$value,$matches)) { |
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.
Is this really necessary?
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.
Yes, otherwise, PhpDumper generates invalid UTF-8 files. As I was unable to properly add safely required characters
| $element->setAttribute('type','expression'); | ||
| $text =$this->document->createTextNode(self::phpToXml((string)$value)); | ||
| $element->appendChild($text); | ||
| }elseif (is_string($value) &&preg_match('/[\x00-\x08\x0b-\x1f\x7f-\xff]/',$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.
What about!preg_match('//u', $value) instead?
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 tried/\p{C}/u but it was not working as expected
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 retested it and tried to detect control characters instead except whitespaces
| $arguments[$key] =newTaggedIteratorArgument($arg->getAttribute('tag')); | ||
| break; | ||
| case'binary': | ||
| if (!preg_match('#^([A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{4}|[A-Za-z0-9+/]{3}=|[A-Za-z0-9+/]{2}==)$#',$arg->nodeValue)) { |
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.
Is that necessary?base64_decode() would returnfalse anyway which we could check for to detect errors, can't we?
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.
Yes, could be done this way also. I generally tend to avoid calling methods with invalid parameters.
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 would side with@xabbuh here: This class should not care about how a valid base64 string looks like. This is an implementation detail of thebase64_decode function.
c43ca2c toe5d9ba8Comparebburnichon commentedJan 25, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've done this PR against master only as it requires xsd to have a new binary definition. But the actual fix can be applied to all versions I think. fabbot.io gives an error in a fixture file. Could this be avoided? |
| $arguments[$key] =newTaggedIteratorArgument($arg->getAttribute('tag')); | ||
| break; | ||
| case'binary': | ||
| if (!preg_match('#^([A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{4}|[A-Za-z0-9+/]{3}=|[A-Za-z0-9+/]{2}==)$#',$arg->nodeValue)) { |
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 would side with@xabbuh here: This class should not care about how a valid base64 string looks like. This is an implementation detail of thebase64_decode function.
derrabus commentedJan 25, 2018
You did not cause the error, so you can ignore it, imho. |
dunglas commentedJan 26, 2018
It deserves a unit test. I would be more comfortable to merge this change in master instead of in 2.7. It's not that trivial. |
bburnichon commentedJan 26, 2018
I made PR against |
bburnichon commentedFeb 9, 2018
xabbuh commentedMar 4, 2018
The CS complaints in the fixtures file can indeed be ignored. But can you please switch back to using explicit |
| break; | ||
| case'binary': | ||
| if (false ===$value =base64_decode($arg->nodeValue)) { | ||
| thrownewInvalidArgumentException(sprintf('Tag "<%s>" with type="binary" is not valid base64 encoded string',$name)); |
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.
[...] is not a valid [...]
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.
Please also terminate the exception message with a dot.
| privatefunctiondoExport($value,$resolveEnv =false) | ||
| { | ||
| if (is_string($value) &&false !==strpos($value,"\n")) { | ||
| if (is_string($value) &&\in_array(preg_match('/[^\s\P{Cc}]/u',$value), [false,1],true)) { |
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.
Is the change for the PHP dumper really necessary? Was there anything breaking without this change?
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.
Not really but then generated file could contain non-utf8 characters and opening generated file with an ide leads to warnings about file encoding.
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.
nothing we need to worry about IMO
| $element->setAttribute('type','expression'); | ||
| $text =$this->document->createTextNode(self::phpToXml((string)$value)); | ||
| $element->appendChild($text); | ||
| }elseif (is_string($value) &&\in_array(preg_match('/[^\s\P{Cc}]/u',$value), [false,1],true)) { |
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.
Why do we need to check for bothfalse and1? Do we cover both values with the current test suite?
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.
false is returned when value contains non-utf8 characters.1 is returned when a control character is present in string. You're right a case is missing in current test suite.
bburnichon commentedMar 5, 2018
Used traditional array syntax and added test on parameter with a single control char. |
af66815 toae1684cCompare| } | ||
| $arguments[$key] =newTaggedIteratorArgument($arg->getAttribute('tag')); | ||
| break; | ||
| case'binary': |
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.
It this really legitimate? I mean: if!!binary is a core yaml feature, it should be transparent. Yaml tagged-values are only for custom tags, not core ones AFAIK.
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 looked for a way to indicate in a XML file (which should be a valid UTF-8 file) that a value wasbase64_encoded(). I though this was the exact same purpose as in yaml file. I did not knew that!!binary was part of the yaml specification.
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.
IMO using the same term here is legitimate. I would keep it as is.
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.
yes, sorry, I misread the code, all good here
| privatefunctiondoExport($value,$resolveEnv =false) | ||
| { | ||
| if (is_string($value) &&false !==strpos($value,"\n")) { | ||
| if (is_string($value) &&\in_array(preg_match('/[^\s\P{Cc}]/u',$value),array(false,1),true)) { |
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.
When can preg_match returnfalse here?
What's the reasoning for choosing this regexp? (any reference/inspiration maybe?)
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.
If$value is not an UTF-8 character string,preg_match will returnfalse because of theu modifier.
Regexp was chosen to detects all characters which are control characters\p{Cc} and not a space (which are valid ascii). I mean, putting a bell character in a string is very unlikely done in purpose. I don't know of a way to put this in a file except by copy/paste.
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.
Hum, I see. Then why unicode at all? No need for theu modifier, isn't it?
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.
If I not setu modifier, I can NOT use\p{Cc} to find the allowed characters as its extension for unicode. My first try was just selecting all characters outside standard ASCII but it would filter too much IMO.
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.
no need for unicode, let's list the ASCII Cc explicitly, here they are:
https://en.wikipedia.org/wiki/Unicode_control_characters
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.
Do you want a different regex for XML and PHP serialization? I mean, in XML, I need to check whether string contains only valid UTF-8 characters implying check with theu modifier.
For PHP, I can only check for[\0-\x1f\x7f] but then simple strings like"Foo\tTab" would be base64 encoded whereas"\xf0" would not.
nicolas-grekasMar 20, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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'd suggest only one way to detect binary, and using this:[\x00-\x08\x0B\x0E-\x1A\x1C-\x1F\x7F]
| privatefunctiondoExport($value,$resolveEnv =false) | ||
| { | ||
| if (is_string($value) &&false !==strpos($value,"\n")) { | ||
| if (is_string($value) &&\in_array(preg_match('/[\x00-\x08\x0B\x0E-\x1A\x1C-\x1F\x7F]/u',$value),array(false,1),true)) { |
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.
no need for theu modifier, thus no need forin_array
nicolas-grekas commentedMar 22, 2018
as bugfix on 3.4? |
fabpot commentedMar 27, 2018
Thank you@bburnichon. |
This PR was squashed before being merged into the 4.1-dev branch (closes#25928).Discussion----------[DI] Allow binary values in parameters.| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#25916| License | MIT| Doc PR | none yetThis adds `binary` type in container xsd definition fileCommits-------cb23134 [DI] Allow binary values in parameters.
stof commentedMar 27, 2018
XML Dumper changes are missing their tests |
stof commentedMar 27, 2018
We should also support it for argument types for consistency |
| $element->setAttribute('type','expression'); | ||
| $text =$this->document->createTextNode(self::phpToXml((string)$value)); | ||
| $element->appendChild($text); | ||
| }elseif (\is_string($value) && !preg_match('/^[^\x00-\x08\x0B\x0E-\x1A\x1C-\x1F\x7F]*+$/u',$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.
Do we still need theu modifier here?
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.
Yes, to ensure string is a valid UTF8 string for XML file.
This PR was merged into the 4.4 branch.Discussion----------[DI] fix parsing of argument type=binary in xml| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets || License | MIT| Doc PR |Parameters and arguments can be of type="binary" since#25928. But the XSD forgot to be updated for arguments.So if you try to run `bin/console debug:container` you'd get an error like```In XmlFileLoader.php line 385: Unable to parse file "var/cache/dev/App_KernelDevDebugContainer.xml": [ERROR 1840] Element '{ht tp://symfony.com/schema/dic/services}argument', attribute 'type': [facet 'enumeration'] The value 'binary' is not a n element of the set {'abstract', 'collection', 'service', 'expression', 'string', 'constant', 'iterator', 'service _locator', 'tagged', 'tagged_iterator', 'tagged_locator'}. (in /home/benny/project/ - line 505, column 0) [ERROR 1824] Element '{http://symfony.com/schema/dic/services}argument', attribute 'type': 'binary' is not a valid value of the atomic type '{http://symfony.com/schema/dic/services}argument_type'. (in /home/benny/project/ - line 5 05, column 0)````Commits-------70b6a00 [DI] fix parsing of argument type=binary in xml
…ers. (vtsykun)This PR was merged into the 5.4 branch.Discussion----------[DependencyInjection] Fix support binary values in parameters.| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#49638| License | MIT| Doc PR | -This issue related to#25928### Step to reproduce1. Add parameter like this```yamlparameters: banner_message: "\e[37;44m#StandWith\e[30;43mUkraine\e[0m"```2. Run command `debug:router`Actual result:Commits-------8541643 Fix support binary values in parameters.
This adds
binarytype in container xsd definition file