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

Support for creating sitemaps #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

spacek
Copy link

@spacek spacek commented Mar 25, 2019

No description provided.

Copy link
Contributor

@janpecha janpecha left a comment

Choose a reason for hiding this comment

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

Díky! Připsal jsem pár dotazů a připomínek. Mimochodem, příště bude určitě lepší pro PR vytvořit samostatnou větev, takhle je součástí PR i přidání EAN do HeurekaFeed, i když je to už mergnuto.

class SitemapItem implements IFeedItem
{

/** @var string|int|NULL */
Copy link
Contributor

Choose a reason for hiding this comment

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

Opravdu může být location typu int?

{
Helpers::assert(isset($this->location), 'Missing item location, call $item->setLocation().');
Helpers::assert(isset($this->lastModified), 'Missing item last modified date, call $item->setLastModified().');
Helpers::assert(isset($this->priority), 'Missing item priority, call $item->setPriority().');
Copy link
Contributor

Choose a reason for hiding this comment

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

Asi bych nevyžadoval nastavení lastModified a priority - podle https://www.sitemaps.org/protocol.html jsou tyto položky nepovinné. A u priority by asi bylo fajn kontrolovat jestli je hodnota (pokud je nastavena) ve správném rozsahu (od 0.0 do 1.0).

* @param string
* @return self
*/
public function setLastModified($lastModified)
Copy link
Contributor

Choose a reason for hiding this comment

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

Co u lastModified místo stringu vyžadovat DateTimeInterface?



/**
* @param string
Copy link
Contributor

Choose a reason for hiding this comment

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

Priorita by IMHO mohla akceptovat kromě stringu i float (resp. number)

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

Successfully merging this pull request may close these issues.

2 participants