prestaconcept / PrestaSitemapBundle

A symfony bundle that provides tools to build a rich application sitemap. The main goals are : simple, no databases, various namespace (eg. google image), respect constraints etc.
MIT License
347 stars 100 forks source link

Resolve bundle deprecations #317

Closed yann-eugone closed 8 months ago

yann-eugone commented 8 months ago

@J-Ben87 do you have an idea about how we could manage a smooth upgrade path with such change https://github.com/prestaconcept/PrestaSitemapBundle/pull/317/commits/10de893665728cb69cc5cd7b963a0c413b74c350#diff-cd1299b45d7a9334149543515cdf5635485b6d18ac3bf7139099f0cc89f6a787R59

J-Ben87 commented 8 months ago

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 :thinking:

yann-eugone commented 8 months ago

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 ?

J-Ben87 commented 8 months ago

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 commented 8 months ago

@J-Ben87 I like your ideas (as always) https://github.com/prestaconcept/PrestaSitemapBundle/pull/319