sulu/SuluArticleBundle

Sitemap generation broken after changing webspace url generation

almare opened this issue · 7 comments

I changed my urls from
<url>{host}/{language}</url>
to
<url language="en">test.local</url> <url language="de">test-de.local</url>
this breaks now my sitemap generation. I receive the following error:

In ArticleSitemapProvider.php line 126:
                                                                                                                                 
  Return value of Sulu\Bundle\ArticleBundle\Sitemap\ArticleSitemapProvider::findUrl() must be of the type string, null returned  

After changing the ArticleSitemapProvider findUrl() method from:

    private function findUrl(
        ArticleViewDocumentInterface $articleView,
        string $scheme,
        string $host,
        string $webspaceKey
    ): string {

to

    private function findUrl(
        ArticleViewDocumentInterface $articleView,
        string $scheme,
        string $host,
        string $webspaceKey
    ): ?string {

the generation works again. It's just a forgotten null return type to fix this issue.

Hey, I am not sure if making the return value nullable is the correct solution here. I feel like this should not be null and something went wrong, if there is no URL for a published article. Is there any valid usecase where this can happen?

Then I have some further questions :)

  • Why is then the result type from the called method null and string?
  • If null is a valid value shouldn't catch findUrl this behavior to prevent breaking the whole sitemap page?

Anyway I guess you are right, that maybe something is wrong here. If you need further informations I will provide them.

Yes, I think the correct implementation would be to check if the value returned by the WebspaceManagerInterface::findUrlByResourceLocator is null and throw a proper exception.

The return value of the WebspaceManagerInterface::findUrlByResourceLocator method is nullable because you can pass an arbitrary $resourceLocator to the method. If there is no Page/Article for that resource locator, the method will return null.

But in the case of the ArticleSitemapProvider, we are iterating through the articles published on the current webspace. I think it is an error, if there is no URL found for such an article. But as said, I am not completely about this. Maybe there is a valid usecase for a published article without an URL.

Thank you for the explanation. At the moment I am happy to get an output without errors. So I will use this as an intermediate solution until the correct behaviour is defined. Maybe it's fixed in the next release 👍

Does the WebspaceManagerInterface::findUrlByResourceLocator method return null for all articles in your case? Does your sitemap contain all your articles at the moment?

After the changes I see all my articles in my sitemap.At the moment there are not so much articles.

Regarding the WebspaceManagerInterface I need to look inside again, but I mean to remember that error occurs because there were also for the german article-sitemap creation the english articles returned. These would be filtered out because the language doesn't match and this would create the null output. I need to verify this, again to get sure.

Closing this because of inactivity. Please create a new issue if this problem still occurs for you 🙂