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

The DualReader can handle only one of them #10

Closed
koriym opened this issue Jul 9, 2021 · 8 comments · Fixed by #11
Closed

The DualReader can handle only one of them #10

koriym opened this issue Jul 9, 2021 · 8 comments · Fixed by #11

Comments

@koriym
Copy link
Owner

koriym commented Jul 9, 2021

@koriym Hi, I came to a bug when only annotations or attributes are used. The DualReader can handle only one of them, depending on PHP version.

This caused missing annotations on PHP 8. I think more reliable solution would be class like this one - that reads both annotations and attributes on PHP 8, if available. That's what I expected from DualReader class and that's where it failed my expectations. But maybe there is a better way to do it.

From contributte/apitte#165 (review)

@TomasVotruba
Copy link
Contributor

I can fix it here... shall I send PR?

@koriym
Copy link
Owner Author

koriym commented Jul 9, 2021

@TomasVotruba Yes! Thanks. But before that let me explain why I was doing it this way.

@TomasVotruba
Copy link
Contributor

No problem. For us it's only important not to break previous behavior, which now happened.

@koriym
Copy link
Owner Author

koriym commented Jul 9, 2021

When supporting PHP8, I assumed that both annotations and attributes would be added as follows.

/**
 * @Foo
 */
#[Foo]
class Fake
{
}

The reason why only one of them is read is for performance. There is no performance loss (PHP source parsing cost) for the dual reader when you start with a mixed library of php7 and php8 attributes and end up with "only" php8 attributes.

This was the way I envisioned it to be used, but I would like to address your case as well.

For this, we need two readers. I would like to change your usage to the default "DualReader" and give the current reader a different name, what do you think?

@koriym
Copy link
Owner Author

koriym commented Jul 9, 2021

Hmm, probably not a good idea.
Let's just make your changes, shall we?

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Jul 9, 2021

When supporting PHP8, I assumed that both annotations and attributes would be added as follows.

In this case I would expected 2 Foo classes. As in these cases:

/**
 * @Foo
 */
#[Foo]
class Fake
{
}
/**
 * @Foo
 * @Foo
 */
class Fake
{
}
#[Foo]
#[Foo]
class Fake
{
}

They are all equal, just with different syntax.

@TomasVotruba
Copy link
Contributor

Hmm, probably not a good idea.
Let's just make your changes, shall we?

I'm on it 👍 :)

@koriym
Copy link
Owner Author

koriym commented Jul 9, 2021

I can fix it here... shall I send PR?

Please do it!

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 a pull request may close this issue.

2 participants