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

[RFC] Rename some function arguments in preparation for PHP 8 #30988

Closed
SharkyKZ opened this issue Oct 8, 2020 · 18 comments
Closed

[RFC] Rename some function arguments in preparation for PHP 8 #30988

SharkyKZ opened this issue Oct 8, 2020 · 18 comments
Labels
No Code Attached Yet PHP 8.x PHP 8.x deprecated issues RFC Request for Comment

Comments

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Oct 8, 2020

Problem identified

With the introduction of named arguments in PHP 8, changing function argument names becomes a B/C break. Until PHP 8 is released, we can update existing function signature with new argument names.

Proposed solution

We should convert existing snake_case arguments to camelCase to better follow our coding standards.
We should review other arguments and rename them if they are unclear or inaccurate.

If not done before PHP 8 is released, the next opportunity would be the next major Joomla! version. That is assuming we do want to add this feature to our B/C promise.

Open questions

Should this be done against staging or 4.0?

@zero-24
Copy link
Contributor

zero-24 commented Oct 8, 2020

Given that 3.9.23 will be the first version eith explizit PHP8 support i would recommend to do any change we want to do now.

Just to avoid any confusion for extension devs. This does not affect any of your extension code yet. Named arguments are opt-in and renaming arguments are only being an issue once named arguments are live. So renaming them now woth the first version that supports PHP8 is fine in my opinion as this is the first version you could build your PHP8 only extension on.

The only very rare case of issue would be that you already have a PHP8 only extension on 3.x that ises named arguments and we are changing just that argument that you where used in your extension. But given no release has benn officially tagged for PHP8 yet and the masshosters do not have PHP8 yet i would say it would be a fair change now to make the future usage consitent.

@zero-24
Copy link
Contributor

zero-24 commented Oct 8, 2020

Cc @HLeithner @wilsonge

@HLeithner
Copy link
Member

I'm in favor for fixing our argument naming but don't think that we should give a b/c promise on named arguments till j4 or later

@Llewellynvdm
Copy link
Member

Making the changes as soon as possible is the wise thing to do. Specially since PHP 8 is due November 26, 2020.

@jiweigert
Copy link

jiweigert commented Oct 9, 2020

Being first time in the front row of implementing new programming language changes of PHP 8 would actually surprise me to see in Joomla, so I'm definitely in favor to see it happen.

@SharkyKZ SharkyKZ added the PHP 8.x PHP 8.x deprecated issues label Oct 12, 2020
@zero-24
Copy link
Contributor

zero-24 commented Oct 17, 2020

I would say after sharing this within all groups and only getting positive feedback I would say we can go for a set of PRs to change the arguments as susggested. @SharkyKZ

@PhilETaylor

This comment was marked as abuse.

@HLeithner
Copy link
Member

No that's not the same or better it's the same because now we have no b/c break for php 8 but for #29812 we would add a b/c break if we did it wrong.

@zero-24
Copy link
Contributor

zero-24 commented Oct 17, 2020

Or better that PR should be send against staging so we can include it in the first release that supports PHP8

@HLeithner
Copy link
Member

sorry my fault I thought the #29812 changes array keys but it doesn't so it should be ok

@zero-24
Copy link
Contributor

zero-24 commented Oct 17, 2020

sorry my fault I thought the #29812 changes array keys but it doesn't so it should be ok

So it should be done against staging right? As when it comes with 4.x it would be a b/c break right?

@PhilETaylor

This comment was marked as abuse.

@zero-24
Copy link
Contributor

zero-24 commented Oct 17, 2020

Well considering some files in #29812 don't even exist in Joomla 3 I don't see how that would be possible :)

Well than they still can be patched with 4.x as this is new API == no B/C break. But the existing ones have to be changed in 3.x before the first official PHP8 supported version.

Plus the lack of interest in #29812 already shows that getting it past the post might be hard :)

Well you know how fast it can go. ;) In this case as this has to be done before the next 3.x release I think we find testers for that too.

@SharkyKZ
Copy link
Contributor Author

PR for libraries/src directory #31148.

@shoulders
Copy link

Can I also refer you to the Database column names which has a camelCase / snake_case issue

joomla/coding-standards#269

@PhilETaylor

This comment was marked as abuse.

@HLeithner
Copy link
Member

@SharkyKZ are we finished with this issue and can we close it?

@HLeithner HLeithner reopened this Nov 21, 2020
@HLeithner
Copy link
Member

I expect that is done, thank you very much @SharkyKZ for creating the PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Code Attached Yet PHP 8.x PHP 8.x deprecated issues RFC Request for Comment
Projects
None yet
Development

No branches or pull requests

8 participants