PhpInternalSourceLocator is not based on internal reflection anymore #470
Conversation
|
Will give it a shot on PHP-Scoper tonight or tomorrow |
|
Works good on my side, thanks a lot @kukulich. For reference, here is the PR: humbug/php-scoper#318 |
kukulich
force-pushed
the
php-internal
branch
7 times, most recently
from
7b48f0a
to
d45d041
Compare
kukulich
force-pushed
the
php-internal
branch
2 times, most recently
from
dbb641b
to
566e509
Compare
| { | ||
| return new class($classReflection->getName()) extends NodeVisitorAbstract | ||
| return new class() extends NodeVisitorAbstract |
There was a problem hiding this comment.
One might say it's not BetterReflection but this piece is a deal-breaker currently for Box. Indeed since it is an anonymous class, this object cannot be serialized which is causing issues when leveraging parallelisation in Box. Would it be possible to move it to a dedicated class? If the API stability is a concern, it could be annotated with a @internal tag
There was a problem hiding this comment.
Uhhh, that is private state of this particular object: why is this breaking Box?
kukulich
force-pushed
the
php-internal
branch
2 times, most recently
from
2617924
to
d789e55
Compare
| { | ||
| return new class($classReflection->getName()) extends NodeVisitorAbstract | ||
| return new class() extends NodeVisitorAbstract |
There was a problem hiding this comment.
Uhhh, that is private state of this particular object: why is this breaking Box?
|
Box executes something along the lines of:
Parallel_map(scope, $filesAndContents);
That function will spawn workers in which “scope” will be called. That
scope function is a static anonymous function which includes the scoper
(itself including BetterReflection). As a result this function is gonna be
serialized & unserialized but anonymous functions cannot be.
…On Tue 14 May 2019 at 12:48, Marco Pivetta ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In bin/generate-phpstorm-stubs-map.php
<#470 (comment)>
:
> + }
+}
+
+ksort($uniqueFiles);
+
+$fileNumbers = array_flip(array_keys($uniqueFiles));
+
+$output = "<?php\n\ndeclare(strict_types=1);\n\n\$files = [\n";
+foreach ($uniqueFiles as $fileName => $tmp) {
+ $relativeFileName = str_replace('\\', '/', substr($fileName, strlen($phpStormStubsDirectory) + 1));
+
+ $output .= sprintf(' \'%s\',%s', $relativeFileName, "\n");
+}
+$output .= "];\n\n";
+
+$output .= "return [\n";
It may as well be ugly, but it works and it will prevent any escaping
issues :P
Let's please not write anything around \\ and stuff like that, and just
use var_export()
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#470?email_source=notifications&email_token=ABHPVAN5YUYPCLKD6PZ36KTPVKKJHA5CNFSM4HLVZVQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBYRNX7A#discussion_r283737351>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABHPVAO65IACAYGOVCZI4Z3PVKKJHANCNFSM4HLVZVQQ>
.
|
|
@theofidry that's a problem that may occur anywhere at any point in time for any of the API of BetterReflection: I don't think that should be discussed in this PR, but I'd also suggest that this should be spawned as an issue in Box itself first. |
|
@Ocramius sure, ideally a more long term plan would be to add support for
anonymous class serialization in amphp/parallel the same way the
serialization for anonymous closures has been added.
However right now there is no issue except for this piece of newly
introduced code which can easily be changed.
…On Tue 14 May 2019 at 12:59, Marco Pivetta ***@***.***> wrote:
@theofidry <https://github.com/theofidry> that's a problem that may occur
anywhere at any point in time for any of the API of BetterReflection: I
don't think that should be discussed in this PR, but I'd also suggest that
this should be spawned as an issue in Box itself first.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#470?email_source=notifications&email_token=ABHPVAK3SQQBL3R354VSXR3PVKLRFA5CNFSM4HLVZVQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVLDUFI#issuecomment-492190229>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABHPVALVRZOE3KEIKKPYUDTPVKLRFANCNFSM4HLVZVQQ>
.
|
|
@theofidry the issue is that anonymous symbols (classes, closures) may be introduced at any time, even in patch releases. Unless explicitly stated, services are not serializable, so there is an architectural issue there, which should not be solved in here. |
| @@ -154,6 +147,11 @@ private function assertSameClassAttributes(CoreReflectionClass $original, Reflec | |||
| $this->assertSameInterfaces($original, $stubbed); | |||
|
|
|||
| foreach ($original->getMethods() as $method) { | |||
| // Needs fix in JetBrains/phpstorm-stubs | |||
There was a problem hiding this comment.
Can we make an issue out of this?
There was a problem hiding this comment.
|
Merging here. @theofidry let's discuss a possible resolution for serialisation in a separate issue/PR |
|
Thanks @kukulich! |
No description provided.