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

PHP: removed built-in type boolean #5294

Merged

Conversation

troizet
Copy link
Collaborator

@troizet troizet commented Jan 14, 2023

What was done in this PR:

@mbien
Copy link
Member

mbien commented Jan 14, 2023

added php label for full test suite. restarting CI.

@apache apache locked and limited conversation to collaborators Jan 14, 2023
@apache apache unlocked this conversation Jan 14, 2023
@KacerCZ
Copy link
Contributor

KacerCZ commented Jan 15, 2023

Proposed change looks good to me.
Maybe then make another PR to remove other type aliases - integer, real, double.

Let's wait for @junichi11's opinion.

@mbien
Copy link
Member

mbien commented Jan 15, 2023

just as reminder: feature freeze for NB 17 is on the 18th, there still is time but not a lot if this is intended for NB 17.

@matthiasblaesing
Copy link
Contributor

Just as a heads up. phpDocumentator explicitly lists:

  • int and integer
  • bool and boolean

but also writes:

The PHPDoc Standard, and thus phpDocumentor, can refer to all primitive types in PHP

This is contradictory and who is right? Can PHPDoc cover more than the "real" primitives or can't it?

@KacerCZ
Copy link
Contributor

KacerCZ commented Jan 15, 2023

PHPDoc types accept also type aliases but these aliases can't be used as native type.
See https://www.php.net/manual/en/language.types.declarations.php#language.types.declarations.base.scalar

This PR fixes case when NetBeans generate type "boolean" and use it as native return type for PHP 7.0 or newer.

@troizet
Copy link
Collaborator Author

troizet commented Jan 16, 2023

I seem to have rushed the PR. Sorry for wasting your time.
It would probably be better if we left the use of boolean type in phpDoc as it is, unchanged. Since boolean in phpDoc is used in most projects.
And remove the built-in boolean type only for type declarations and type guessing.

@junichi11
Copy link
Member

junichi11 commented Jan 16, 2023

It would probably be better if we left the use of boolean type in phpDoc as it is, unchanged. Since boolean in phpDoc is used in most projects.

Agree. It would be better.
As @matthiasblaesing wrote, I guess it was added for phpDocumenter.

@junichi11 junichi11 requested a review from tmysik January 16, 2023 06:45
@troizet troizet marked this pull request as draft January 16, 2023 07:33
@tmysik
Copy link
Member

tmysik commented Jan 16, 2023

If I get it right, the ideal situation here would be this one:

  • in the PHP code, use bool everywhere; but
  • keep support for boolean, but only in the PHPDoc comments (kind-of like a legacy code, we would not generate it, but still were able to read/understand it).

Does it make sense? Would it be possible to implement it?

@troizet troizet force-pushed the php_remove_built_in_type_boolean branch 2 times, most recently from 9f873ba to cf5698c Compare January 29, 2023 05:05
@troizet
Copy link
Collaborator Author

troizet commented Jan 29, 2023

Updated the PR so that it does not affect the phpDoc.

@troizet troizet marked this pull request as ready for review January 29, 2023 06:33
tmysik
tmysik previously approved these changes Jan 30, 2023
Copy link
Member

@tmysik tmysik left a comment

Choose a reason for hiding this comment

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

The change looks good to me, but please, let @junichi11 approve it as well. Thank you.

@junichi11
Copy link
Member

@troizet Could you improve the commit message like the title and description of this PR? (We should add related issue numbers.)
If you would like to fix this into NB17, please rebase on delivery branch. (cc: @mbien)

@neilcsmith-net
Copy link
Member

@junichi11 I'm taking that as you making the call that it belongs in delivery! 😄 Adding milestone and do-not-merge pending rebasing of the PR so we can track. 17-rc3 is the last guaranteed release candidate unless we have urgent fixes, so ideally needs to be in a position I can merge within the next 24hrs.

@neilcsmith-net neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 30, 2023
@neilcsmith-net neilcsmith-net added this to the NB17 milestone Jan 30, 2023
  - deleted the built-in type boolean
    (php does not have a built-in boolean type: https://www.php.net/manual/en/language.types.intro.php
    Also see the warning under "Scalar Types": https://www.php.net/manual/en/language.types.declarations.php)
  - fixed broken tests
  - fixed issues:
    - PHP: Generates the return type boolean instead of bool for a method when overriding a base class method with an autocomplete apache#5283,
    - PHP: Generates the return type boolean instead of bool for a function in phpDoc apache#5284
  - new tests for fixed issues have been added
  - does not affect phpDoc
@troizet troizet force-pushed the php_remove_built_in_type_boolean branch from cf5698c to 3436a10 Compare January 30, 2023 16:19
@troizet troizet changed the base branch from master to delivery January 30, 2023 16:19
@troizet troizet dismissed tmysik’s stale review January 30, 2023 16:19

The base branch was changed.

@troizet
Copy link
Collaborator Author

troizet commented Jan 30, 2023

I changed the commit message and rebased it on delivery. @junichi11, @neilcsmith-net please check if I did it right?

Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Thank you for your work!

@junichi11
Copy link
Member

@neilcsmith-net Thank you for your help :)

@tmysik The release team (@neilcsmith-net ) merges this.

BTW, CI doesn't work, right?

@apache apache locked and limited conversation to collaborators Jan 30, 2023
@apache apache unlocked this conversation Jan 30, 2023
@neilcsmith-net neilcsmith-net removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 31, 2023
@neilcsmith-net
Copy link
Member

neilcsmith-net commented Jan 31, 2023

Thanks! Looks good, merging.

BTW, CI doesn't work, right?

@junichi11 can be manually triggered by locking and unlocking the conversation as above if needed. It's a "hidden" feature!

@neilcsmith-net neilcsmith-net merged commit a54e469 into apache:delivery Jan 31, 2023
@troizet
Copy link
Collaborator Author

troizet commented Jan 31, 2023

Thanks!

@junichi11
Copy link
Member

can be manually triggered by locking and unlocking the conversation as above if needed. It's a "hidden" feature!

@neilcsmith-net I see. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PHP [ci] enable extra PHP tests (php/php.editor)
Projects
None yet
7 participants