Sitemap support #152
Sitemap support #152
Conversation
sweoggy
commented
Jun 15, 2018
sweoggy
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | fixes #41 |
| License | MIT |
- Added SitemapPlugin to application test and dev environments - Added tests of provider
|
Wow! Thank you a lot! @patrick477, can I ask you to review it? |
| $pageUrl = $this->sitemapUrlFactory->createNew(); | ||
| $pageUrl->setChangeFrequency(ChangeFrequency::daily()); | ||
| $pageUrl->setPriority(0.7); | ||
| if ($page->getUpdatedAt()) { |
There was a problem hiding this comment.
Doesn't $page always have an updated at? Else fall back to created at maybe?
$pageUrl->setLastModification($page->getUpdatedAt() ?? $page->getCreatedAt());
| } | ||
| /** @var PageTranslationInterface $translation */ | ||
| foreach ($this->getTranslations($page) as $translation) { | ||
| if (!$translation->getLocale()) { |
There was a problem hiding this comment.
getLocale() can return null, this is just a safeguard against that.
There was a problem hiding this comment.
@sweoggy I did you CR, it would be great if you would like to relate to this
| ->getQuery() | ||
| ->getResult(); | ||
| } | ||
|
|
There was a problem hiding this comment.
a better way would be to add the findEnabled method to the repository
| * @var LocaleInterface | ||
| */ | ||
| protected $locale2; | ||
|
|
There was a problem hiding this comment.
a better name would be $secondLocale
| */ | ||
| private function getLocaleCodes(): array | ||
| { | ||
| if ($this->channelLocaleCodes === null) { |
There was a problem hiding this comment.
a better practice would be null === $this->channelLocaleCodes
| * | ||
| * @return SitemapUrlInterface | ||
| */ | ||
| private function createProductUrl(PageInterface $page): SitemapUrlInterface |
There was a problem hiding this comment.
should this method be named createPageUrl?
| } | ||
| return $pageUrl; | ||
| } | ||
|
|
There was a problem hiding this comment.
run the bin/ecs check src --fix command to correct the coding standard
|
Thanks @stefandoorn @patrick477 for your reviews, I have made changes accordingly 😃 |
| @@ -19,15 +18,20 @@ | |||
| "friends-of-behat/service-container-extension": "^1.0", | |||
| "friends-of-behat/symfony-extension": "^1.0", | |||
| "friends-of-behat/variadic-extension": "^1.0", | |||
| "lakion/api-test-case": "^1.1", | |||
There was a problem hiding this comment.
I upgraded it to ^2.0|^3.1.0 last week, which went fairly easy.
There was a problem hiding this comment.
Surely we would need to upgrade to PHPUnit ^6.0 at least for the whole plugin then? Feels like that belongs to a separate PR.
|
Looks good to me :) |
|
@sweoggy Thank you a lot! 🙂 |
