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

Resolve bundle deprecations #317

Merged
merged 5 commits into from
Oct 9, 2023
Merged

Resolve bundle deprecations #317

merged 5 commits into from
Oct 9, 2023

Conversation

yann-eugone
Copy link
Member

No description provided.

@yann-eugone yann-eugone self-assigned this Oct 9, 2023
@yann-eugone
Copy link
Member Author

@J-Ben87 do you have an idea about how we could manage a smooth upgrade path with such change
10de893#diff-cd1299b45d7a9334149543515cdf5635485b6d18ac3bf7139099f0cc89f6a787R59

@J-Ben87
Copy link
Member

J-Ben87 commented Oct 9, 2023

You mean the fact that the parameter is no longer nullable? Isn't this already handled by the dependency injection and the service definition we have control over?
And if you mean the fact that the arguments have been reordered, and in case someone would have overridden the service definition, I guess most people are now using named arguments so that shouldn't be too much of a problem.
Otherwise I guess you might try something with a compiler pass that could maybe guess the arguments types and reorder them (or warn if one is missing) and replace the service definition 🤔

@yann-eugone
Copy link
Member Author

A parameter not being nullable anymore is not really an issue, because you can add a if to trigger deprecation
A parameter being reordered is much of an issue, and I've no idea how to warn users about it
I believe only doing it in a major version and adding a release note might be enough ?

public function __construct(
EventDispatcherInterface $dispatcher,
int $itemsBySet = null,
UrlGeneratorInterface $urlGenerator = null
UrlGeneratorInterface $urlGenerator
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ you forgot to reorder arguments here

@J-Ben87
Copy link
Member

J-Ben87 commented Oct 9, 2023

That, or using non typed arguments and warning if their type is invalid.
Ex.

public function __construct(
    EventDispatcherInterface $dispatcher,
    Filesystem $filesystem,
    $urlGenerator,
    $sitemapFilePrefix = Configuration::DEFAULT_FILENAME,
    $itemsBySet = null
) {
    parent::__construct($dispatcher, $itemsBySet, $urlGenerator);

    if (
        is_string($urlGenerator)
        && (is_int($sitemapFilePrefix) || is_null($sitemapFilePrefix))
        && $itemsBySet instanceof UrlGeneratorInterface
    ) {
        $tmpUrlGenerator = $itemsBySet;
        $itemsBySet = $sitemapFilePrefix;
        $sitemapFilePrefix = $urlGenerator;
        $urlGenerator = $tmpUrlGenerator;

        // warn about the reordering
    }

    if (
        !is_string($sitemapFilePrefix)
        || (!is_int($itemsBySet) && null !== $itemsBySet)
        || !$urlGenerator instanceof UrlGeneratorInterface
    ) {
        throw new InvalidArgumentException();
    }

    $this->filesystem = $filesystem;
    $this->sitemapFilePrefix = $sitemapFilePrefix;
}

@yann-eugone
Copy link
Member Author

@J-Ben87 I like your ideas (as always)
#319

*/
public function __construct(
UrlContainerInterface $urlContainer,
string $section = null,
UrlGeneratorInterface $urlGenerator = null
UrlGeneratorInterface $urlGenerator
Copy link
Member

Choose a reason for hiding this comment

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

Woops, we also missed this one 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch

@yann-eugone yann-eugone merged commit 27227a8 into 4.x Oct 9, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants