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

Added public methods to make Sitemap model plugin friendly #9970

Merged
merged 1 commit into from
Jun 24, 2017

Conversation

7ochem
Copy link
Contributor

@7ochem 7ochem commented Jun 16, 2017

Description

I have added two public methods to the \Magento\Sitemap\Model\Sitemap to be able to add other 'sitemap items' via a plugin.

I encountered this problem when I had a third party module already overriding (via preference) the Sitemap model and I ended up doing a classical rewrite conflict fix.

Plugins could prevent rewrite conflicts, but the class that is the subject of these plugins should have public methods to target.

I added the public methods collectSitemapItems() and addSitemapItem(). I kept the protected method _initSitemapItems() in the code (instead of replacing _initSitemapItems() with collectSitemapItems()) to maintain backwards compatibility for extensions that already do rewrite the whole \Magento\Sitemap\Model\Sitemap model and override the _initSitemapItems() to be able to add sitemap items.

Fixed issue/PR

  1. Alter Sitemap's items with results from new event #9530: Alter Sitemap's items with results from new event

Manual testing scenarios

  1. Add a plugin targeting the Sitemap model (<type name="Magento\Sitemap\Model\Sitemap"><plugin ... /></type>)
  2. Add an afterCollectSitemapItems(\Magento\Sitemap\Model\Sitemap $subject) method
  3. Use the \Magento\Sitemap\Model\Sitemap::addSitemapItem(\Magento\Framework\DataObject $sitemapItem) method to add new sitemap items
  4. Generate a sitemap. This should now also contain your custom sitemap URLs

Example:

class SitemapPlugin {
    /**
     * Add custom sitemap items
     * @param \Magento\Sitemap\Model\Sitemap $subject
     */
    public function afterCollectSitemapItems(\Magento\Sitemap\Model\Sitemap $subject) {
        $sitemapItem = new \Magento\Framework\DataObject([
            'changefreq' => $this->getSitemapChangeFrequency($storeId),
            'priority' => $this->getSitemapPriority($storeId),
            'collection' => /** ... insert array of \Magento\Framework\DataObject here ... */,
        ]);
        $subject->addSitemapItem($sitemapItem);
    }
}

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jun 16, 2017

CLA assistant check
All committers have signed the CLA.

@avoelkl
Copy link
Contributor

avoelkl commented Jun 20, 2017

Hi @7ochem,
Nice PR! Will have a look!

@avoelkl avoelkl self-assigned this Jun 20, 2017
@avoelkl avoelkl added this to the June 2017 milestone Jun 21, 2017
@avoelkl
Copy link
Contributor

avoelkl commented Jun 21, 2017

Hi @7ochem, I am happy to report all the internal integration tests are green :)

Copy link
Contributor

@antonkril antonkril left a comment

Choose a reason for hiding this comment

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

We discourage methods that mutate object state.

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.

@okorshenko
Copy link
Contributor

Hi @7ochem Could you please look at @antonkril comment. thank you

@7ochem
Copy link
Contributor Author

7ochem commented Jun 22, 2017

@avoelkl @antonkril @okorshenko thanks for your time looking into my proposal.

I think I know what you mean with the resolver. I'm a bit conflicted here though. The sitemap module seems pretty straight forward (1 model only, no data models, no generators), so I was going for a simple fix.

Now you are asking for a level of complexity that doesn't fit the (direct) context. If we'd go for an item resolver, or a pool, then we would better go for a more up scaled refactoring involving actual models for the items (instead of arrays of DataObjects) and adding more "single responsibility" by having a separate class for generating the actual XML output, leaving the old Sitemap model more of a manager service orchestrating the process.

What are your thoughts?

@antonkril
Copy link
Contributor

It would probably be over-complication, but that is out of the scope of this PR.

I am talking about replacing one customization point with other, more error-proof.

@avoelkl
Copy link
Contributor

avoelkl commented Jun 23, 2017

Hi @7ochem @antonkril @okorshenko,
How can we proceed to get this PR merged?
I suggest we add a @todo tag to the methods with the information that this will be refactored later and add a new issue on GitHub with the "Up for grabs" label.
Maybe @7ochem or someone else wants to pick this up later.
I really like this PR as it is a backwards compatible improvement to add items to the sitemap.

@7ochem
Copy link
Contributor Author

7ochem commented Jun 23, 2017

@antonkril, thank you for your clarification. I sometimes find it difficult to estimate what level of complexity will be accepted by the Magento team (what level of complexity you are after).

I agree with @avoelkl to see if my current suggestion could be merged (as it is an improvement already to the existing situation) and try to improve this afterwards in the way @antonkril suggests.

I'd rather take small steps at a time than creating 1 big PR, if that is ok with you?

@okorshenko okorshenko self-assigned this Jun 23, 2017
@antonkril
Copy link
Contributor

antonkril commented Jun 23, 2017

Usually, I would recommend our developers not to merge it, but the state of Sitemap is pretty simple. So if you don't have time/do not want to modify the code - ok, let's merge it. It is, definitely, an improvement over the current state.

@avoelkl
Copy link
Contributor

avoelkl commented Jun 25, 2017

Thanks for merging, I just added a new issue with the "up for grabs" label to improve this even more :)

@7ochem
Copy link
Contributor Author

7ochem commented Jun 26, 2017

So @avoelkl do you perhaps know where this will end up? Still in 2.2? Or 2.2.1?

@okorshenko
Copy link
Contributor

Hi @7ochem and @avoelkl this fix will be available with 2.2.0 release

@7ochem
Copy link
Contributor Author

7ochem commented Jun 30, 2017

@okorshenko should I also create a PR for 2.1.x-develop? So we have this improvement also into 2.1 LTS.

Eventually #10045 will probably only be created for 2.2 as it is somewhat a new feature

@okorshenko
Copy link
Contributor

it make sense to implement #10045 and then backport to 2.1

@korostii
Copy link
Contributor

korostii commented Jul 3, 2017

Hi @7ochem,

also into 2.1 LTS

Sorry for the off-topic, but was it actually announced anywhere that 2.1 will have long-term support?
I heard just the opposite recently (security fixes only)

@7ochem
Copy link
Contributor Author

7ochem commented Jul 3, 2017

Hi @korostii, indeed slightly off topic ;)

I gathered some info here once: https://magento.stackexchange.com/a/131008/3326

I did not run into a good definition of the actual scope for LTS releases. Certainly interesting with a new minor release coming shortly. What will this mean for upcoming patch releases for previous minor releases...

Anyway, this thread might not be the right place for such a discussion. You should probably open up another issue here on GitHub, or head to the Magento forums or the Magento Stackexchange.

@7ochem 7ochem deleted the sitemap-model-plugin-friendly branch August 26, 2019 12:43
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.

7 participants