BookStackApp / BookStack

A platform to create documentation/wiki content built with PHP & Laravel
https://www.bookstackapp.com/
MIT License
15.43k stars 1.94k forks source link

Add OpenSearch support #5198

Closed maximilian-walter closed 1 month ago

maximilian-walter commented 2 months ago

Add XML-file and HTML meta-tag so browsers can autodiscover the search of BookStack instances. See:

https://developer.mozilla.org/en-US/docs/Web/OpenSearch

Closes BookStackApp/BookStack#5122

ssddanbrown commented 2 months ago

Thanks for offering this @maximilian-walter!

Here's my feedback from a quick review (have not actually pulled down and tested yet):

Are you happy to take on these changes? Just let me know if you need further help/guidance, or if you're not able to perform further work for any reason.

maximilian-walter commented 2 months ago

Thanks @ssddanbrown for your feedback!

Can the images be reduced down to just the 32 & 64? Can't see evidence of large images being used here, and I'd prefer to keep things to what's required/used where possible.

I don't know if higher resolutions would be used. I would expect that, but I did not validate it. Because we have them already I would keep them, but I am also fine if you wish to reduce the number of links.

The URL has a method attribute, but I can't see this as part of the spec?

You are right, I removed the attribute. We don't need it anyway.

The ShortName must be limited to 16 characters or less.

The hardlimit of 16 characters is now enforced.

The Description is lacking translation support.

I moved the description to the language file. I hope this was the correct way of doing it in regards of Crowdin.

We'd want to have some tests to cover this, (Simple example of tests for licenses endpoint).

I added some basic test based on your HtmlTest-class. Because the Symfony Crawler is used, I assumed it is fine to feed XML to it. :)

A proper XML schema validation would be much better, but I did not find any DTD file for this standard.

ssddanbrown commented 1 month ago

Thanks @maximilian-walter! Could you just also restore the two .gitignore files removed in this PR? BTW, I'll probably squash merge this to contain the feature to one commit, and prevent files being removed/re-added in git history if that's okay.

maximilian-walter commented 1 month ago

@ssddanbrown Sorry, I added the missing files again and rebased my branch so the files do not get removed and added again. Squashing is still a good idea. I used multiple small commits only for a easier code review. :)

ssddanbrown commented 1 month ago

Now merged for next feature release, thanks again @maximilian-walter!

While testing I found that chrome would totally ignore opensearch discovery if not hosting on the root of the domain, whereas Firefox was totally happy with that. Will have to remember this for future potential support scenarios!