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

Google Sitemap refactoring - introduce item resolver #10288

Merged
merged 14 commits into from
Jul 27, 2017

Conversation

piotrkwiecinski
Copy link
Contributor

@piotrkwiecinski piotrkwiecinski commented Jul 19, 2017

Refactoring to introduce injectable item resolver as way to add items to sitemap

Description

  • Introduce SitemapItemResolverInterface with concrete implementations for cms pages, products, categories and aggregate
  • Introduce SitemapItemInterface as representation of single sitemap item
  • Introduce SitemapConfigReaderInterface
  • Introduce SitemapItemResolverConfigInterface
  • Remove addSitemapItems as it was introduced in develop branch and wasn't used yet as public API
  • Deprecate helper (suggested by @vrann). Getting data from configuration is part of introduced concrete implementations.

Fixed Issues (if relevant)

  1. Refactoring: Adding items to the Sitemap #10045: Refactoring: Adding items to the Sitemap

Manual testing scenarios

  1. Go to Marketing > SEO > Sitemap
  2. Add sitemap - provide path and filename
  3. Click generate & save sitemap
  4. Check if content is properly generated.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@okorshenko okorshenko added this to the July 2017 milestone Jul 19, 2017
@okorshenko
Copy link
Contributor

Hi @piotrkwiecinski Could you please look at broken tests?

@piotrkwiecinski
Copy link
Contributor Author

@vrann, @okorshenko tests should be green now. Last one was static.

@piotrkwiecinski piotrkwiecinski changed the title [WIP]Google Sitemap refactoring - introduce item resolver Google Sitemap refactoring - introduce item resolver Jul 24, 2017
@vrann
Copy link
Contributor

vrann commented Jul 25, 2017

@piotrkwiecinski finished code review, looks good. Running internal builds.

@vrann vrann self-requested a review July 25, 2017 21:15
@okorshenko okorshenko self-assigned this Jul 26, 2017
}
}

/**
* Add a sitemap item to the array of sitemap items
Copy link
Contributor

Choose a reason for hiding this comment

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

Please restore these two public methods:

  • they are in the class marked as @api
  • the class will be released as is in 2.2 release
  • current pull request will get to the 2.3 release

@magento-team magento-team merged commit 458d45b into magento:develop Jul 27, 2017
magento-team pushed a commit that referenced this pull request Jul 27, 2017
@piotrkwiecinski piotrkwiecinski deleted the sitemap-item-resolver branch July 27, 2017 21:04
@Ctucker9233
Copy link

@magento-team Will this get backported to 2.2?

@piotrkwiecinski
Copy link
Contributor Author

If it would be useful and there is no objections from @magento-team I could port it back on Monday or Tuesday

@Ctucker9233
Copy link

@piotrekkaminski Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants