Repository navigation
Support new in initializers - #966
Conversation
|
The static analysis failure is my fault, I guess. 🙈 See #967. |
8f43d78 to
bcfaaa8
Compare
|
@derrabus I knew I've seen that PR somewhere, couldn't recall where :D thanks for creating a fix so quickly 🍻 |
|
Please rebase, static analysis should be green now. 🤞🏻 |
bcfaaa8 to
205401e
Compare
|
@nicolas-grekas Can you have a look, please? |
| } | ||
|
|
||
| /** | ||
| * @return string |
There was a problem hiding this comment.
Why not use a native type declaration here?
There was a problem hiding this comment.
No particular reason, I was mimicking other methods in the generator.
There was a problem hiding this comment.
Let's deal with those globally later? We will be able to add them to private methods only though
There was a problem hiding this comment.
doctrine/common is probably going to die soon, so that's really not a big deal, we can deal with it later or not at all.
There was a problem hiding this comment.
I'm referring to a discussion on a private Slack.
| continue; | ||
| } | ||
|
|
||
|
$part = preg_replace('/(?(DEFINE)(? |
There was a problem hiding this comment.
What is the purpose of this line? When I comment it out, the test below does not fail 🤔 … I'm using PHP 8.1.8 Likewise, if I put rubbish in the regex line 1133, no test fails.
There was a problem hiding this comment.
I have absolutely no clue, I took what @nicolas-grekas provided for ProxyManager for granted 🙈 Tried to go for original FriendsOfPHP PR but nobody challenged that there either
There was a problem hiding this comment.
Then let's just return what's on line 1132? It seems to be the important part of this patch. When somebody reports the bug the bottom part fixes, let's make sure to add a test that documents it, as well as comments.
There was a problem hiding this comment.
This regexp is supposed to add the \ prefix to internal classes when they declare a default value that references a class constant.
There was a problem hiding this comment.
This sounds like it would also affect top-level classes that are not internal, but user defined, wouldn't it? In both cases, I'm unsure if doctrine/common should support it, as AFAIK it's only ever used to proxify entities and documents, that should never be in the top-level namespace, or inherit from a class defined in it. Unlike proxy-manager I suppose.
There was a problem hiding this comment.
That would be quite weird but I not completely impossible I suppose. @malarzm , I think we should either add a test case based on ArrayObject::asort() (which refers to a constant), or drop the code.
There was a problem hiding this comment.
Added part with using global const, it does fail without those regexes :)
There was a problem hiding this comment.
Really? When I try again what I did in #966 (comment) on eb0aaa5, the test still passes 🤔
Also, dumping $part shows no prepended backslash.
There was a problem hiding this comment.
Really?
Dang, sorry. I made an early return in getDefaultValue that was supposed to break the test... it did, but for totally other reason :s
There was a problem hiding this comment.
Sadly, I'm giving up. I'm almost starting my vacations and I think it'll be better to have main cases covered. If somebody reports a bug that could be fixed by regexes I've just removed then it can be done in a bugfix manner.
| } | ||
|
|
||
| $value = rtrim(substr(explode('$' . $parameter->getName() . ' = ', (string) $parameter, 2)[1], 0, -2)); | ||
| $parts = preg_split('{(\'(?:[^\'\\\\]*(?:\\\\.[^\'\\\\]*)*)\')}', $value, -1, PREG_SPLIT_DELIM_CAPTURE); |
There was a problem hiding this comment.
It would be nice to use the x delimiter and add comments inside that regex. If that makes it become multiline, then maybe also use nowdoc? That way there won't be a need for escaping the single quotes.
205401e to
eb0aaa5
Compare
eb0aaa5 to
70a835e
Compare
70a835e to
a45d437
Compare
5 participants
Port of Ocramius/ProxyManager#755
Fixes #961