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

[Bug]: Return type error when using Ignition Runnable Solutions #5

Closed
duncanmcclean opened this issue Jul 9, 2024 · 2 comments · Fixed by #6
Closed

[Bug]: Return type error when using Ignition Runnable Solutions #5

duncanmcclean opened this issue Jul 9, 2024 · 2 comments · Fixed by #6
Labels
bug Something isn't working

Comments

@duncanmcclean
Copy link
Contributor

duncanmcclean commented Jul 9, 2024

What happened?

We have a few "runnable solutions" in our package, implementing the legacy Ignition contracts.

For context, here's one of the exceptions we're returning a runnable solution from:

<?php

namespace Statamic\Exceptions;

use Exception;
use Spatie\Ignition\Contracts\ProvidesSolution;
use Spatie\Ignition\Contracts\Solution;
use Statamic\Ignition\Solutions\EnableStatamicPro;

class StatamicProRequiredException extends Exception implements ProvidesSolution
{
    public function getSolution(): Solution
    {
        return new EnableStatamicPro;
    }
}

We've just had a user report an error due to the getSolution method using an incorrect return type.

Previous to the recent solutions refactor, the RunnableSolution interface (which in our case, is implemented by the EnableStatamicPro class) extended the Solution interface, which meant the Solution return type would work.

However, after the solutions refactor, the legacy RunnableSolution interface now extends the new RunnableSolution interface, and it extends the new Solution contract.

Since it's no longer returning the legacy Solution contract (which we're using as our return type), users who hit that exception will see an error about the incorrect return type:

Statamic\Exceptions\StatamicProRequiredException::getSolution(): Return value must be of type Spatie\Ignition\Contracts\Solution, Statamic\Ignition\Solutions\EnableStatamicPro returned

One possible fix for this issue would be to re-add the relevant methods to the legacy RunnableSolution interface, then have it extend the legacy Solution interface, to fix the return type error.

Alternativley, on our end, we could just update the return type on our end to expect RunnableSolution, instead of Solution. However, this wouldn't fix the issue for other apps/packages implementing their own runnable solutions.

How to reproduce the bug

  1. Create a fresh Laravel app
  2. Require spatie/laravel-ignition (which in turn, requires this package)
  3. Create a runnable solution, which implements the legacy RunnableSolution interface
  4. Create an exception, which implements the ProvidesSolution interface with a getSolution method. All imports pointing to the legacy Ignition namespace.
  5. See a return type error when the exception is thrown

For ease, I've just ran through these steps for you and pushed up an example repo: https://github.com/duncanmcclean/spatie-ignition-runnable-solutions-return-type-error

Package Version

1.0.4

PHP Version

8.3.0

Which operating systems does with happen with?

macOS

Notes

Related: statamic/cms#10416

@patinthehat
Copy link

Hey @duncanmcclean, are you able to open a PR with a fix and unit tests or a PR with failing unit tests? Thanks for reporting this issue! 👍

@duncanmcclean
Copy link
Contributor Author

Hey @duncanmcclean, are you able to open a PR with a fix and unit tests or a PR with failing unit tests? Thanks for reporting this issue! 👍

Done, see #6.

@riasvdv riasvdv closed this as completed in #6 Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants