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

Add support for php 8 attributes #64

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

xico42
Copy link
Contributor

@xico42 xico42 commented Oct 25, 2021

Refactor the current schema generation test case in order to reuse the
same logic for schema generation with php 8 attributes.

Create a new schema attribute reader for extracting schema info from php
attributes.

Once nested attributes is supported only in php 8.1, this feature supports only php >= 8.1.

Refactor the current schema generation test case in order to reuse the
same logic for schema generation with php 8 attributes.

Create a new schema attribute reader for extracting schema info from php
attributes.

namespace FlixTech\AvroSerializer\Objects\Schema\Generation\Attributes;

enum Type: string
Copy link

Choose a reason for hiding this comment

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

That would require to bump min. version to 8.1, right ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here, min version for this library is PHP 7.4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily. These enums are being used only by the "attributes reader" code, so if one does not use that than php 8.1 won't be required, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, the files get parsed for static code analysis and code coverage. We would have to ignore them individually for each of these tools, and I am not even sure if that is possible.

Copy link
Contributor Author

@xico42 xico42 Nov 2, 2021

Choose a reason for hiding this comment

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

I've found some ways of doing so by conditionalling excluding those files when the php version is not supported. I've prepended these commits as WIP just for testing puprposes. It workerd locally, let's see how it goes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tPl0ch when you are able to, please take a look at this commit: ec7314c

Copy link
Collaborator

@tPl0ch tPl0ch left a comment

Choose a reason for hiding this comment

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

The attributes are fine, but we should support both (old annotations and attributes) until we would drop PHP 7.4 as a requirement. The enums unfortunately need to go.


namespace FlixTech\AvroSerializer\Objects\Schema\Generation\Attributes;

enum Order: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

We cannot support this yet in this library, we are supporting PHP 7.4 here


namespace FlixTech\AvroSerializer\Objects\Schema\Generation\Attributes;

enum Type: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here, min version for this library is PHP 7.4

 - Refactor unit tests in order to remove php 8.1 specific tests from
 older php versions
 - Conditionally remove or add code to static analysis based on the
 current php version
 - Update minimum phpunit version
@@ -33,6 +33,10 @@ jobs:
-
version: 8.0
xdebug: 3.1.1

-
version: 8.1.0RC3
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version: 8.1.0RC3
version: 8.1.0

Now that 8.1 is out

@@ -6,6 +6,13 @@
->in(['src', 'test'])
;

if (version_compare(PHP_VERSION, '8.1') < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we make it work on 8.0 if we don't use nested attributes?

@AlexOstrovsky
Copy link

AlexOstrovsky commented Apr 21, 2023

Any update on this? It would be nice to use PHP attributes instead of annotations.

@tPl0ch
Copy link
Collaborator

tPl0ch commented Jan 26, 2024

@AlexOstrovsky, I'd be happy if someone takes this PR as a base and adds the required changes. Also, PHP 7.4 is dropped, so it would make things easier. As said before, I am currently not able to do this.

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.

5 participants