search

magento / community-features

Magento Features Development is an Initiative to Allows Community Memebers Join to Development of Magento Features
46 stars 18 forks source link

SitemapItem implements dto pattern inconsistent with the rest of the framework. #91

Open deefco opened 6 years ago

deefco commented 6 years ago

Summary

In Magento 2.3 Sitemap has been split into Sitemap and SitemapItem, so we can use ItemProviders to compose sitemaps. The way SitemapItem has been implemented is kind of weird though, it is a pure DTO. Yet is doesn't extend DataObject or any of it's child classes, all data is passed through the constructor and saved in class properties.

The issue with this is that ItemProviders now simply use new SitemapItem() calls meaning it is very hard to extend SitemapItems with more data in a clean way.

Proposed solution

Make SitemapItem extend DataObject (or any of it's children), replace class properties with $data and interface all constants used.

thomas-kl1 commented 6 years ago

Because it's not the right way to extend it. You should play with the Magento\Sitemap\Model\ItemProvider\Composite and Magento\Sitemap\Model\SitemapItemInterface. If you want to override or extend an existing sitemap item, you are free to override the item via the di.xml: see https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Sitemap/etc/di.xml

deefco commented 6 years ago

How does the existence of the Compose provider and the SitemapItemInterface mean data objects should no longer extend DataObject?

sivaschenko commented 3 years ago

DTOs should not extend DataObject, however, for extensibility purposes, the DTO interfaces should extend \Magento\Framework\Api\ExtensibleDataInterface.

I am not sure about the need to provide extensibility for sitemap item, though.

Can you please share a use case?