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

Fix type-safety for ExtensionPluginManager #88

Open
wants to merge 1 commit into
base: 2.23.x
Choose a base branch
from

Conversation

RobLoach
Copy link
Contributor

This fixes a few type definitions that were missing when defining instances within ExtensionPluginManager, resulting in a complete white-screen when error_reporting(E_ALL) is enabled.

For instance, PluginManagerInterface::validate expects to return void.

This change fixes the following...

Got error 'PHP message: PHP Fatal error: Declaration of Laminas\Feed\Reader\ExtensionPluginManager::validate(mixed $instance) must be compatible with Laminas\ServiceManager\PluginManagerInterface::validate(mixed $instance): void in /var/www/html/vendor/laminas/laminas-feed/src/Reader/ExtensionPluginManager.php on line 203'

Q A
Documentation yes
Bugfix yes
BC Break no
New Feature no
RFC no
QA yes

Description

  • Are you fixing a bug or providing a failing unit test to demonstrate a bug? Yes, this fixes type safety issues in the inherited class.

    • How do you reproduce it? Use error_reporting(E_ALL), and inherit from laminas-service 4.3
    • What did you expect to happen? For there to not be any type errors.
    • What actually happened? It reported the above error
    • TARGET THE CURRENT RELEASE BRANCH
  • Are you adding documentation? No.

    • TARGET THE CURRENT RELEASE BRANCH
  • Are you providing a QA improvement (additional tests, CS fixes, etc.) that
    does not change behavior? No, there is no behavior change.

    • Explain why the changes are necessary. The change is required for type-safety across the PHP classes.
    • TARGET THE NEXT MINOR BRANCH
  • Are you fixing a BC Break? Unsure.

    • How do you reproduce it? Use error_reporting(E_ALL)
    • What was the previous behavior? Things work without the error reporting.
    • What is the current behavior? Things break when you're using error reporting.
    • TARGET THE CURRENT RELEASE BRANCH
  • Are you adding something the library currently does not support? No.

  • Are you refactoring code? No.

This fixes a few type definitions that were missing when defining instances within `ExtensionPluginManager`, resulting in a complete white-screen when `error_reporting(E_ALL)` is enabled.

For instance, [`PluginManagerInterface::validate`](https://github.com/laminas/laminas-servicemanager/blob/4.3.x/src/PluginManagerInterface.php#L27) expects to return `void`.

Signed-off-by: Rob Loach <robloach@gmail.com>
@RobLoach
Copy link
Contributor Author

The NonInvariantPropertyType errors should be present in the parent classes as well.

@Xerkus
Copy link
Member

Xerkus commented Sep 16, 2024

Adding types to properties and return types would be a BC break.

This package is not compatible with service manager v4. It is an optional dependency so no composer constraints. Conflict with v4 should be declared that can be resolved only in the next major.

@RobLoach
Copy link
Contributor Author

Thanks for the explanation, Xerkus. Are there plans to make it compatible with v4? Anything I can do to help make it happen?

@Xerkus
Copy link
Member

Xerkus commented Sep 16, 2024

A new major version is needed but it can't really be done in multiple small steps. Before it can be tagged a major QA overhaul to modernize code to the current PHP, improve types and psalm coverage, resolve any other breaking issues and so on.

If you are willing to tackle those tasks we can start work on a new major release.

@froschdesign
Copy link
Member

froschdesign commented Sep 16, 2024

@RobLoach
The ExtensionPluginManager is optional, you can use the StandaloneExtensionManager instead.

And an update is planned, but first all the foundational components need to be updated.

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

Successfully merging this pull request may close these issues.

3 participants