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

Incorrect ReflectionClass::getNamespaceName for anonymous class #12895

Open
kubawerlos opened this issue Dec 7, 2023 · 11 comments · May be fixed by #12907
Open

Incorrect ReflectionClass::getNamespaceName for anonymous class #12895

kubawerlos opened this issue Dec 7, 2023 · 11 comments · May be fixed by #12907

Comments

@kubawerlos
Copy link

Description

The following code:

<?php

namespace Foo {
    interface InterfaceFoo {}
}

namespace Bar {
    new class implements \Foo\InterfaceFoo {};
}

namespace {
    foreach (get_declared_classes() as $class) {
        if (!str_contains($class, '@anonymous')) {
            continue;
        }
        echo 'Class: ', $class, PHP_EOL;
        echo 'Namespace: ', (new ReflectionClass($class))->getNamespaceName(), PHP_EOL;
    }
}

Resulted in this output:

Class: Foo\InterfaceFoo@anonymousD:\a\php-anonymous-class-namespace\php-anonymous-class-namespace\test.php:8$0
Namespace: Foo\InterfaceFoo@anonymousD:\a\php-anonymous-class-namespace\php-anonymous-class-namespace

at Windows and

Class: Foo\InterfaceFoo@anonymous/home/runner/work/php-anonymous-class-namespace/php-anonymous-class-namespace/test.php:8$0
Namespace: Foo

at Ubuntu

But I expected this output instead:

Class: Foo\InterfaceFoo@anonymousD:\a\php-anonymous-class-namespace\php-anonymous-class-namespace\test.php:8$0
Namespace: Bar

Reproduced at https://github.com/werlos/php-anonymous-class-namespace (see the only action there).

PHP Version

8.3.0

Operating System

No response

@iluuu1994
Copy link
Member

iluuu1994 commented Dec 7, 2023

Hi @kubawerlos. I don't understand, why do you expect backslashes for paths on Ubuntu? The slashes are not namespace separators, its a path appended to the class name to avoid naming conflicts of anonymous classes. The class name of anonymous classes is essentially an implementation detail.

Edit: Oh, sorry. I missed that this is talking about getNamespaceName, not the slashes.

@nielsdos
Copy link
Member

nielsdos commented Dec 7, 2023

This is because Windows paths use backslashes, but the function also uses backslashes to determine where to "cut off" the string:

const char *backslash = zend_memrchr(ZSTR_VAL(name), '\\', ZSTR_LEN(name));
if (backslash) {
RETURN_STRINGL(ZSTR_VAL(name), backslash - ZSTR_VAL(name));
}

Tested on my Windows 10 VM I get the same behaviour as OP.

@nielsdos
Copy link
Member

nielsdos commented Dec 7, 2023

And getShortName is also messed up as it'll return "foo.php:8$0".

@iluuu1994
Copy link
Member

iluuu1994 commented Dec 7, 2023

Indeed. I think OP would also expect Unix to yield Bar, but because the name is prefixed with the parent class (or first implemented interface) rather than the namespace it's also wrong.

@nielsdos
Copy link
Member

nielsdos commented Dec 7, 2023

Okay, so fixing this would consist of two parts:

  • Fix how a prefix is chosen such that the correct namespace is included in the name
  • If on Windows, and ZEND_ACC_ANON_CLASS is set, check the position of @ and search for the backslash before that

WDYT?

@iluuu1994
Copy link
Member

That sounds reasonable. namespace@anonymouspath:line$counter should do. I don't know what the point of including the parent class is.

@nielsdos
Copy link
Member

nielsdos commented Dec 7, 2023

I don't know what the point of including the parent class is.

72bd559

"This is intended to display a more useful class name in error messages
and stack traces, and thus make debugging easier."

I guess it doesn't really matter that much, but I did find this.

@nielsdos
Copy link
Member

nielsdos commented Dec 7, 2023

@iluuu1994 How do you want to proceed here? I have a working Windows development environment and I already know how to tackle the second bullet point.

EDIT: or we could split the issue between us: 2nd bullet point for me and 1st for you? Just let me know so we at least don't do double work ;)

@iluuu1994
Copy link
Member

@nielsdos However you want, I'll focus on something else tomorrow. I'm certainly not mad if you take care of either or both. ^^ I don't think this is high priority though.

@nielsdos
Copy link
Member

nielsdos commented Dec 7, 2023

Okay, will tackle this tomorrow.

nielsdos added a commit to nielsdos/php-src that referenced this issue Dec 8, 2023
…ymous class

Because paths can contain backslashes, searching for the first backslash
starting on the right for detecting namespaces yields the wrong result.
Furthermore, because the namespace of the class is not actually in the
name (only the interface or parent class is) the namespace can be wrong.

This fix consists of two parts:
 - Fix how a prefix is chosen such that the correct namespace is included in
   the name.
 - If ZEND_ACC_ANON_CLASS is set, check the position of @ and search for the
   backslash before that.

While the issue seems Windows-specific, I could even reproduce this on Linux
because Linux allows directories to contain backslashes.
@Wirone
Copy link

Wirone commented Mar 19, 2024

Let's code PHP like there's no tomorrow! 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants