Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] PhpDumper should follow coding standards#20082
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4d71286 tob7291f5Comparero0NL commentedSep 28, 2016
Im not sure this is a real issue... imo. codestyle doesnt really apply to generated code anyway. |
2d9a0e6 to7cf4772Comparejaviereguiluz commentedSep 29, 2016
I'm 👍 I know that these generated files are critical and we shouldn't mess with them; these files are generated for computers, not humans; etc. But the proposed changes look safe and Symfony is a project that should strive for top quality in everything it does, like generating these files. |
linaori commentedSep 29, 2016
We'll always be 1 step behind of the codestyle of those files. If it's really important to have them properly formatted, it should be using an up-to-date code formatter (php-cs-fixer?). Imo if readability of generated files is really an issue, I'd rather see them being formated on 1 line and use your IDE or a custom cs fixer on demand when required. |
| protectedfunctionexportArray($array) | ||
| { | ||
| returnpreg_replace(array('/array \(/','/,\n *\)/','/,\n */','/\n */'),array('array(',')',',',''),var_export($array,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.
there is a risk of collision with the content of a string here, CS is not worth it imho
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.
You are right. I thought I had this covered, but I must have missed something (I wrote this code a while back, so I don't remember my reasoning).
I cannot think of an easy way to fix this, so I think I'll just close this PR.
BTW the existing code simply strips newlines – this is also not completely kosher, but it might be enough for the specific arrays in question.
| protectedfunctionexportArray($array) | ||
| { | ||
| returnpreg_replace(array('/array \(/','/,\n *\)/','/,\n */','/\n */'),array('array(',')',',',''),var_export($array,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.
collision risk again?
While working on#19562 I noticed that Fabbot does not like the PHP code generated by the
PhpGeneratorDumperandPhpMatcherDumper.This patch makes the generated code comply with Symfony's conding standards.