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

Refactoring: Adding items to the Sitemap #10045

Closed
avoelkl opened this issue Jun 25, 2017 · 10 comments
Closed

Refactoring: Adding items to the Sitemap #10045

avoelkl opened this issue Jun 25, 2017 · 10 comments
Assignees
Labels
Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release up for grabs

Comments

@avoelkl
Copy link
Contributor

avoelkl commented Jun 25, 2017

PR #9970 was already an improvement to the sitemap for easier adding of additional items.

@antonkril suggested to refactor the improved behaviour even more:

Expected result

Better approach in this case would be to introduce the reversed extension point: some kind of $ItemResolver dependency that would be called by sitemap (with sitemap id argument) to load custom sitemap items.

This dependency could be implemented as a composite, or just pluginized by third-party developers to add new items to the custom list.

@piotrkwiecinski
Copy link
Contributor

Here is my proposal:

interface SitemapItemResolverInterface {
    /**
      * @return SitemapItemInterface[]
      */
    public function getItems();
}

To create CmsPageSitemapItemResolver,CategorySitemapItemResolver,ProductSitemapItemResolver as implementation of above. In addition create CompositeSitemapItemResolver with constructor parameter SitemapItemResolverInterface $itemResolvers = []

DI preference for SitemapItemResolverInterface would be CompositeSitemapItemResolver

interface SitemapItemInterface {
    public function getUrl();

    public function getPriority();

    public function getChangeFrequency();

    public function getImages();

    public function getUpdatedAt();
}

As method addSitemapItem() was added only to develop and item resolvers are more flexible solution I would prefer to remove it.

I'm happy to work on it.

@vrann
Copy link
Contributor

vrann commented Jul 14, 2017

@piotrekkaminski proposal looks good, go for it!

@avoelkl
Copy link
Contributor Author

avoelkl commented Jul 18, 2017

@piotrkwiecinski see last message :) guess the wrong piotr was notified :)

piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 19, 2017
Add injectable item resolver to load sitemap items
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 19, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 19, 2017
@antonkril
Copy link
Contributor

"Resolver" is not the best name here. "List" or "Provider" would be a better fit.

piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 23, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 23, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 23, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 24, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 24, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 24, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 25, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 26, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 26, 2017
piotrkwiecinski added a commit to piotrkwiecinski/magento2 that referenced this issue Jul 26, 2017
@magento-team magento-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development develop up for grabs labels Jul 31, 2017
@magento-team
Copy link
Contributor

Internal ticket to track issue progress: MAGETWO-70872

@piotrkwiecinski
Copy link
Contributor

@antonkril I haven't noticed your comment. I could provide additional PR as a follow up to change resolver to provider. WDYT @vrann?

@magento-team
Copy link
Contributor

Internal ticket to track issue progress: MAGETWO-71372

@magento-engcom-team magento-engcom-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development develop Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed up for grabs labels Sep 11, 2017
@magento-engcom-team magento-engcom-team added Fixed in 2.3.x The issue has been fixed in 2.3 release line Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Sep 20, 2017
@magento-engcom-team
Copy link
Contributor

@avoelkl, thank you for your report.
The issue is already fixed in develop branch
But we will consider to backport the fix to patch releases

@magento-engcom-team
Copy link
Contributor

Hi @avoelkl
Can we close this issue or there are some tasks TODO?

@avoelkl
Copy link
Contributor Author

avoelkl commented Oct 9, 2017

Hi @magento-engcom-team, sorry I missed the notification. This issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release up for grabs
Projects
None yet
Development

No branches or pull requests

6 participants