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

feat: add support for PHP 8.4 #567

Merged
merged 2 commits into from
Nov 4, 2024
Merged

feat: add support for PHP 8.4 #567

merged 2 commits into from
Nov 4, 2024

Conversation

@@ -83,7 +85,8 @@ public function for(callable $function): FunctionDefinition
*/
private function signature(ReflectionFunction $reflection): string
{
if (str_contains($reflection->name, '{closure}')) {
// PHP8.4 Only `str_starts_with($name, '{closure:')` is needed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With 8.2 you can just use ReflectionFunction::isAnonymous().

@nickvergessen
Copy link
Contributor

I guess you don't feel comfortable to release a version without Psalm CI on your side, so you have a proof.
But would be helpful for your consumers already to be able to check and test things going forward 🙈

@romm romm force-pushed the feat/php-8.4 branch 8 times, most recently from 4e448d5 to 0038849 Compare November 1, 2024 16:10
@romm
Copy link
Member Author

romm commented Nov 2, 2024

Hi @TimWolla sorry to poke you here, but as you fixed the ReflectionFunction::inNamespace() issue in php/php-src#16129 I thought I could ask for some help. Hope you have time. 😊

It seems that even with the latest PHP 8.4-RC, we're still having an issue with \CuyZ\Valinor\Tests\Integration\Mapping\Namespace\NamespacedInterfaceInferringTest::test_interface_inferred_from_same_namespace_as_file_runs_correctly ; the issue lies in \CuyZ\Valinor\Type\Parser\Factory\Specifications\AliasSpecification::resolveNamespaced(): the closure is namespaced but the method still returns false.

Any idea what is wrong here? Thanks!

@TimWolla
Copy link
Contributor

TimWolla commented Nov 2, 2024

@romm Closures are never considered namespaced with PHP 8.4+. Generally speaking you can't really rely on a specific closure naming, because Closures are anonymous and thus you would be relying on an implementation detail. From how the PHP code looked like, even the namespacing of Closures prior to 8.4 was an accident (they are just named {closure} and the namespace was implicitly added like for any other function). There weren't any tests either.

@romm
Copy link
Member Author

romm commented Nov 3, 2024

@TimWolla thanks for your answer.

The usecase here is to get the namespace of a closure so that short class names in docblock can be transformed to FQCN. For instance:

namespace SimpleNamespace;

use CuyZ\Valinor\MapperBuilder;

interface SomeInterface {}

final class ImplementationOne implements SomeInterface {}

final class ImplementationTwo implements SomeInterface {}

return (new MapperBuilder())
    ->infer(
        SomeInterface::class,
        /** @return class-string<ImplementationOne|ImplementationTwo> */
        static fn (string $type): string => match ($type) {
            'one' => ImplementationOne::class,
            'two' => ImplementationTwo::class,
            default => throw new \RuntimeException(),
        },
    )
    ->mapper()
    ->map(
        SomeInterface::class,
        ['type' => 'one'],
    );

The annotation @return class-string<ImplementationOne|ImplementationTwo> should understand that ImplementationOne should be fetched from namespace SimpleNamespace.

It seems strange to me that closures cannot be namespaced, is there any chance the old behavior could be reverted and tested? If not, would you have a suggestion on how to get this information? Using PhpToken::tokenize on the source file content and fetching the correct namespace?

@TimWolla
Copy link
Contributor

TimWolla commented Nov 3, 2024

It seems strange to me that closures cannot be namespaced

Namespaces are not a first class citizen in PHP. It's really just compiler assisted automated prefixing of any names. The getNamespaceName() methods in Reflection really do just that: Extract the part before the last backslash.

is there any chance the old behavior could be reverted and tested?

Unfortunately not for PHP 8.4, it's much too late in the release cycle.

If not, would you have a suggestion on how to get this information?

The effect on docblock parsing is an unfortunate side-effect of the improved closure naming that I did not anticipate when initially implementing it.

Using PhpToken::tokenize on the source file content and fetching the correct namespace?

For any closure that is not a top-level closure you can strip the {closure: prefix and then find the last backslash. Anything in-between is the namespace. For top-level closures (i.e. closures without any surrounding function or method) you would need to tokenize the file, I'm afraid.

@romm
Copy link
Member Author

romm commented Nov 4, 2024

Thank you @TimWolla for the details. Tokenization was not that hard, and all this stuff gets cached anyway (if the cache is used), so that's not a big deal.

@romm romm merged commit 07a06a2 into CuyZ:master Nov 4, 2024
14 checks passed
@romm romm deleted the feat/php-8.4 branch November 4, 2024 09:00
@TimWolla
Copy link
Contributor

TimWolla commented Nov 4, 2024

@romm FWIW: The tokenization solution is not quite correct in all cases, because the following is legal PHP code:

<?php
namespace Foo {
 use Bar\Bar;
 $closure = function () { var_dump(new Bar()); };
 class Foo { }
 var_dump($closure());
}

namespace Bar {
 use Foo\Foo;
 $closure = function () { var_dump(new Foo()); };
 class Bar { }
 var_dump($closure());
}

namespace {
 $closure = function () { };
}

if (! $reflection->inNamespace()) {
if ($reflection->inNamespace()) {
$namespace = $reflection->getNamespaceName();
} elseif ($reflection instanceof ReflectionFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could guard this against closures to not unnecessarily tokenize the file for top-level functions in non-namespaced code.

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.

3 participants