[PHP] Return type changed in PHP >= 8-0. Fluent setters now return static.#9431
[PHP] Return type changed in PHP >= 8-0. Fluent setters now return static.#9431DamImpr wants to merge 1 commit into
static.#9431Conversation
DamImpr
changed the title
static.static.
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
In general makes sense to me. I left an inline comment, regarding an unintentional change?!
@tmysik could you please have a look?
|
@matthiasblaesing I will look into it tomorrow. |
| } | ||
|
|
||
| private String getReturnType() { | ||
| if (cgsInfo.isFluentSetter() && cgsInfo.getPhpVersion().hasStaticReturnType()) { |
There was a problem hiding this comment.
In order to use this newly added API method, you need to update the dependency version (once it is increased).
There was a problem hiding this comment.
I don't think I've quite got it. I've changed the version code in the "hasStaticReturnType" method.
Is that enough?
There was a problem hiding this comment.
Well, this was already discussed with @neilcsmith-net and currently, we don't version our APIs on every change (basically, no API version change in the development cycle). So, what exact version should be set in the Javadoc, I don't know. Neil, could you advise, please? Perhaps the current version + 1?
@DamImpr how this is supposed to work if we version every API change:
- update/change the code of the API module (
php.api.phpmodulehere) - because of this change, increase the specification version of this module (e.g.
3.107 -> 3.108) - you want to use this newly added code in the PHP editor, so
- update the dependency version in
php.editormodule to the latest one:3.108 - now, the newly added code is visible also in the PHP editor
Hopefully, it is more clear now.
There was a problem hiding this comment.
The current version in master is fine IMO. It is already ahead of NB30.
| } | ||
|
|
||
| private String getReturnType() { | ||
| if (cgsInfo.isFluentSetter() && cgsInfo.getPhpVersion().hasStaticReturnType()) { |
There was a problem hiding this comment.
Please, provide a link with documentation of this change. Does it mean that every fluent getter in PHP 8+ is static? Or am I wrong?
There was a problem hiding this comment.
Exactly! I don’t know where to find a link to the documentation right now, but fluent setters always return an instance of the object itself. What’s more, the ‘insert code’ feature already ensured that fluent setters returned $this; all I added was the return type, which had previously been omitted in the case of fluent setters.
There was a problem hiding this comment.
@DamImpr, I am afraid that without official documentation of this change, we cannot merge this PR, sorry. We need to stay correct and as I wrote, it seems to me that with this change, every fluent setter would become a static one. And that is quite unexpected, no?
tmysik
left a comment
tmysik
left a comment
There was a problem hiding this comment.
Overall, it looks good to me, thank you for your work.
Just as requested in one of my comments, please add a link to the official PHP documentation describing this feature. This is always useful. Once we verify it, we are good to merge (once my comments are fixed too).
Thank you!
|
thank you @tmysik thank you |
DamImpr
force-pushed
the
fluent_setter_static_from_php_80
branch
from
b4c1031 to
e812432
Compare
|
@matthiasblaesing @tmysik |
|
@DamImpr, thanks for your changes, but see my comment above - we need the official PHP documentation of this feature/change so we are correct from the implementation/support point of view. Thank you for understanding. |
|
@tmysik Let me have a look in the official PHP documentation, in the section on the |
5 participants
From PHP version 8.0 onwards, the "static" return type has been adopted for fluent setters. In the "Generate Code" menu in NetBeans, no return type was assigned when fluent setters were created. This PR ensures that fluent setters, when used in a project configured with PHP version >= 8.0, correctly have the "static" return type.