Skip to content

Added Sitemap generator#2610

Closed
bridgeyuwa wants to merge 8 commits intoBookStackApp:masterfrom
bridgeyuwa:master
Closed

Added Sitemap generator#2610
bridgeyuwa wants to merge 8 commits intoBookStackApp:masterfrom
bridgeyuwa:master

Conversation

@bridgeyuwa
Copy link

Added a feature which will automatically generate and update sitemap to improve search engine optimization

One step into the SEO optimization of BookStack.

@ssddanbrown
Copy link
Member

Thanks for offering this PR @bridgeyuwa.

Personally I feel this implementation would raise some considerable concerns for me:

  1. It introduces an additional, complex dependency to the project.
  2. The mechanism this performs the sitemap generation is not something I'd feel comfortable including in the project due to past security considerations I've seen from server-side requests. If we had to implement such a feature I'd prefer a database-driven solution.
  3. I'm not really comfortable with use of the ini_set functions, I'd prefer to stand by system settings.
  4. This introduces scheduling which will need to be an additional thing to document and support.

Overall, this implementation does not really integrate much with the project itself, You could essentially wrap the library used into another app then run that against your BookStack URL to achieve the same result. Otherwise there are various services/commands to help generate sitemaps in a similar manner.

This is one of those features that I don't think there's enough demand to really warrant the build and effort into the project at this stage. Now we have the API, Sitemap generation could be a good use for that for the people that do feel they need it.

@bridgeyuwa
Copy link
Author

bridgeyuwa commented Mar 4, 2021

OK..
From what you said about preferring a database driven solution, I think I have an Idea of using a Sitemap Controller to generate a dynamic xml view . but I am skeptical about it because of the nested logic I will have to use in the view to fetch the database relationships.

Maybe I'll do it the best I can and see if anyone is willing to help fix/refactor anything that isn't right, e.g redundancy.
I am still hopeful for a solution.

@ssddanbrown
Copy link
Member

@bridgeyuwa Okay, I'd still be worried about introducing a hefty feature, especially since it's not highly requested. It may get quite complex and tricky once you start considering permissions.

I've created a simplistic example of using the API to create a sitemap in our API examples repo here:
https://github.com/BookStackApp/api-scripts/tree/main/php-generate-sitemap
It will generate for all primary content URLs. The code is commented in an attempt to be easy to read and easily modified where required.

@bridgeyuwa
Copy link
Author

Wonderful Job well-done

How about making the lastmod get its value from updated_at column of the pages table instead of $nowDate

on line 99 ===> $url->appendChild($doc->createElement('lastmod', $nowDate));

@ssddanbrown
Copy link
Member

@bridgeyuwa From searching yesterday most sources tended to say the lastmod is generally ingored nowdays, but I've updated the script to use item updated_at values.

@bridgeyuwa
Copy link
Author

This is perfect.

@ssddanbrown
Copy link
Member

Cool, I'll therefore close off this PR and it's implementation.

@ssddanbrown ssddanbrown closed this Mar 7, 2021
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