-
Notifications
You must be signed in to change notification settings - Fork 5
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 sitemap for collections & taxonomies #78
base: master
Are you sure you want to change the base?
Conversation
src/Commands/GenerateSitemap.php
Outdated
|
||
return $terms | ||
->filter(function ($term) { | ||
return view()->exists($term->template()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you check for a template in the query? It's a bit more efficient, also don't have to check the is not empty then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #78 (comment)
Taxonomies are limited in the way we can check for urls, Statamic seems to be taking this approach themselves as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not what i'm saying, you can skip the whole ->filter()
if you doe a ->where
on the template in the query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No thats not possible, the template is not something we can query on. You can see why here https://github.com/statamic/cms/blob/5.x/src/Taxonomies/LocalizedTerm.php#L381
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in the taxonomy_terms
table there is a uri
column as with rapidez/statamic
we require the Eloquent driver so with that we don't have to check for existing views? When the view doesn't exist that uri
column should be empty according https://statamic.dev/taxonomies#routing, didn't test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was just able to test it, unfortunately an existing uri
column doesn't guarantee an existing view. It seems like as long as a term has a slug, the uri
field will be filled.
This is probably also why Statamic does it this way themselves in their SEO addon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, still added the uri check in the query. Since all terms that do have views also do have uri's.
src/Commands/GenerateSitemap.php
Outdated
{ | ||
$sitemap = '<?xml version="1.0" encoding="UTF-8"?>'; | ||
$sitemap .= '<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" xmlns:xhtml="http://www.w3.org/1999/xhtml">'; | ||
$entries = $collection->queryEntries()->where('site', $site->handle())->whereStatus('published')->get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have collections that have no route, like Category content, but have no url, how is this handled? I see no check on the ->url()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The collections that are queried are already filtered by collections that have urls, so collections like the category content won't come here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is checking the view the right way? Can't we check if the collection/taxonomy has a route?
Sadly we cant check routes on taxonomies the same way we can check routes on collections. |
Co-authored-by: Roy Duineveld <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When generated they aren't ignored by the .gitignore
by default I think?
Correct, i added a line in the readme for this now! |
@@ -2,7 +2,6 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
De we maybe need to schedule the jobs? So we don't have to manually run this every time a entry get's created.
src/Jobs/GenerateTermSitemapJob.php
Outdated
{ | ||
$sitemap = $this->sitemapGenerator->createSitemap($this->generateTermSitemap()); | ||
|
||
file_put_contents(public_path('sitemap_statamic_taxonomy_' . $this->site->handle() . '_' . $this->taxonomy->handle() . '.xml'), $sitemap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safer to use the Laravel Storage facade for this
@@ -28,7 +29,7 @@ public function __construct( | |||
public function handle(): void | |||
{ | |||
$sitemap = $this->sitemapGenerator->createSitemap($this->generateCollectionSitemap()); | |||
file_put_contents(public_path('sitemap_statamic_collection_' . $this->site->handle() . '_' . $this->collection->handle() . '.xml'), $sitemap); | |||
Storage::disk('public')->put('sitemap_statamic_collection_' . $this->site->handle() . '_' . $this->collection->handle() . '.xml', $sitemap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the public path always the same as the public disk?
@@ -213,6 +218,41 @@ public function bootUtilities() : static | |||
return $this; | |||
} | |||
|
|||
public function bootSitemaps(): static | |||
{ | |||
Schedule::command('rapidez:statamic:generate:sitemap')->twiceDaily(0, 12); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we want to force this from the package, maybe put it somewhere in the readme? Or provide a command from rapidez/sitemap
where this package hooks into. So you'll just get 1 command which you can schedule yourself which generates all sitemaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this around so it hooks into rapidez/sitemap#2.
See: #78 (comment)
} | ||
|
||
|
||
protected function generateCollectionSitemap() : string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not place this in the action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep the logic for getting the collection entries in the job, same goes for getting the terms in the term job. The action itself is just for building the sitemap based on the given urls.
Co-authored-by: Bob Wezelman <[email protected]>
This PR is now depending on rapidez/sitemap#2 for it to generate the sitemaps. Although it's still possible to run the command manually, this PR would essentially schedule the sitemap generation to twice per day. |
"rapidez/blade-directives": "^0.6", | ||
"rapidez/core": "^2.13", | ||
"justbetter/statamic-glide-directive": "^2.1", | ||
"rapidez/sitemap": "^1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be raised when rapidez/sitemap#2 is merged and released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better that rapidez/sitemap
provides this so rapidez/statamic
can use it. Any other packages which needs to register a sitemap could also benefit from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe remove it and provide an option from rapidez/sitemap
:
rapidez:sitemap:generate
= allrapidez:sitemap:generate statamic
rapidez:sitemap:generate anything-else
And register new sitemaps with Eventy with a key so that can be used here.
} | ||
} | ||
|
||
protected function addSitemap($url = null) : string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also move this to rapidez/sitemap
and use that here.
No description provided.