Support PHP 8.4 #44
Support PHP 8.4 #44
Conversation
|
Lots of changes, so that easy to get lost. I wished next time, we can have smaller changes. Good to see all tests are passing. |
There was a problem hiding this comment.
@harikt thanks for your review, please see my comments.
If you need this broken up somehow, I'm happy to do that with some direction.
Otherwise, I would appreciate it if you would merge and release, as all tests and all CI actions in supported PHP versions are passing.
@harikt It does not break BC, as demonstrated by the passing tests. It might affect implementations that have extended classes to overload any of these methods, but that is not a documented use case and I don't think you need to be concerned with it. If it were me I would do a minor version release, I don't think it merits a major version release. |
|
@compwright as we are not type hinting return type on other methods can we remove the new type defined
No you don't need to do in this PR. I was just telling smaller changes are easy to review and move forward. I will do a release once you make the change. |
| @@ -66,10 +69,10 @@ public function testNoForeignType() | |||
|
|
|||
| $this->manager = new Manager($type_builder, $relation_builder, $types); | |||
| $this->expectException('Aura\Marshal\Exception'); | |||
| $this->manager->posts; | |||
| $this->manager->__get('posts'); | |||
There was a problem hiding this comment.
The effect is the same, the change makes the code more readable, and satisfies PHPStan.
There was a problem hiding this comment.
I do understand the effect is same, but wished those changes are not made to satisfy phpstan. This is tests file right ? I can revert that, not an issue.
There was a problem hiding this comment.
To be honest, it should never have been written the first way. It is a statement that only has any meaning BECAUSE there is a magic method, but that is not obvious to look at it. It looks like a typo, and PHPStan flags things like that for good reason. Calling the magic method explicitly is exactly what should be done in a test case for the magic method.
My 2 cents :)
There was a problem hiding this comment.
@compwright It's up to @harikt to resolve the conversation when he sees fit.
Thank you for the suggestion. However, since we haven't marked the class as final, adding a runtime void return type poses a risk of breaking existing subclasses. We believe @return void alongside phpstan is sufficient in this case, and don't see a compelling reason to introduce a runtime void at this point. |
|
Thank you @compwright for the changes. I am ok with the changes. Anyone else have a different opinion? I will wait for 2 more days before merging. |
Will you be able to release this soon? Thanks in advance! |
|
@compwright https://github.com/auraphp/Aura.Marshal/releases/tag/4.0.3 . Sorry for the delay and thanks for making this happen. Regarding the usage Another thing as @pmjones mentioned, the person who have asked the question should resolve the conversation, else it will get lost. Thank you all for your help. |
Resolves #43