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 type restrictions to class methods #6759

Open
marc-farre opened this issue Dec 18, 2023 · 3 comments
Open

Add type restrictions to class methods #6759

marc-farre opened this issue Dec 18, 2023 · 3 comments

Comments

@marc-farre
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

Since PHP 7.4.0 we can add return types restrictions to class methods.
So we should add them everywhere.
But if another class extends the class where a return type has been added, we have a "PHP Compile Error – yii\base\ErrorException".

So each time a new return type restriction is added, it's documented on https://github.com/humhub/humhub/blob/develop/MIGRATE-DEV.md#type-restrictions

Problem: even if well documented, it requires to check all the modules and update them.

If return types restrictions are added over time, we need to do this work several times (checking all the code in modules and publish new versions).
Moreover, we might have more and more Humhub instances having troubles if they didn't update all modules before updating Humhub.

Describe the solution you'd like

Adding all types restrictions at once. One PR for Humhub core containing all these added restrictions and then it's done, we don't need to add more gradually later.

Describe alternatives you've considered

Create a documentation of all return types restrictions that will be added in a future Humhub version.
This way, we can anticipate this change by releasing a new module version already having these type restrictions.

@luke-
Copy link
Contributor

luke- commented Dec 18, 2023

Related: #6754

@luke-
Copy link
Contributor

luke- commented Dec 18, 2023

The current approach would be to define return values only very carefully in methods that are rarely or not used by modules.

When it comes to the module API, we should go with a tool like Rector.

@martin-rueegg
Copy link
Contributor

As mentioned here, for fields there is no progressive migration. It needs to happen in one go:

The problem still exists for class fields, though. There we do not have the ability to enforce the type later (as with function parameters) or earlier (as with function return types). a work-around would be to remove overwriting parameters in child classes and instantiate the values first thing in the constructor instead.

If the updater is not already doing this, I guess it should

  • check every installed module for it's compatibility with the to-be-installed core version and the user can decide whether to do the upgrade and suspend the module, or to wait for module update
  • check if there is an updated version of the module on the marketplace and offer to update it together with the core.

Might be something for

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants