Make BrowserShot Optional in SitemapGenerator

[antonioribeiro]

Summary of the Change

This pull request proposes making BrowserShot and the Crawler functionality optional in the SitemapGenerator package. The current implementation forces these dependencies even for users who don’t require dynamic crawling, introducing security risks and compatibility challenges.

Given the recent security issue in BrowserShot, along with its PHP version requirements, and the additional unnecessary dependencies, this change aims to improve flexibility and reduce overhead for users of the package.

Why This Change is Necessary

1. Security Concern

The BrowserShot package had a security issue that was patched in version 5. However, the patched version requires PHP 8.2. Many applications, including ours, are still running on PHP 8.1, leaving them unable to use the patched version and exposed to vulnerabilities in BrowserShot v3.

2. Unnecessary Dependencies

Currently, Spatie Laravel Sitemap pulls in several dependencies that are only required for the Crawler feature. For users who generate sitemaps directly (without crawling), these dependencies add unnecessary overhead, increasing maintenance complexity and potential attack surfaces.

3. Symfony DomCrawler Note

We noticed that symfony/dom-crawler is explicitly required in the composer.json of spatie/laravel-sitemap, but it appears to be unnecessary as it is already required by spatie/crawler.

👉 Reference: spatie/crawler composer.json

4. Our Use Case

In our projects, we generate sitemaps directly from routes and database queries. We do not use the crawling functionality, yet we are forced to install these dependencies, which are never actually used in our application.

Removed Dependencies

By making BrowserShot and Crawler optional, the following packages will no longer be required:

Package
masterminds/html5
nicmart/tree
spatie/browsershot
spatie/crawler
spatie/robots-txt
spatie/temporary-directory
symfony/dom-crawler

Benefits of This Change

1. Reduced Security Risk
   Users who don’t require dynamic crawling won’t be exposed to potential vulnerabilities in these packages if any arise in the future.

2. Reduced Overhead
   Removing unnecessary dependencies results in a lighter application footprint, faster builds, and less complexity.

3. Increased Flexibility
   Users can choose to install BrowserShot and Crawler only if their use case requires it.

Proposed Change

As the SitemapGenerator class is already conditionally using BrowserShot only when the package is configured to use the Crawler, we propose to:

• Move the dependencies from the required section to the suggest on the composer.json file.
• Add the spatie/crawler to the require-dev section on the composer.json file to allow testing.
• Update the package documentation to clarify that these features are optional and provide guidance on how to enable them if needed.
• Review the symfony/dom-crawler dependency to ensure it is truly necessary in spatie/laravel-sitemap.

Closing Thoughts

We highly value the contributions of Spatie to the Laravel community. This proposed change aims to improve the Laravel Sitemap package by making it more flexible and secure while reducing unnecessary dependencies for users who don’t need the Crawler functionality.

We appreciate your consideration of this pull request and look forward to any feedback. Thank you for your continued efforts to enhance the Laravel ecosystem!

[freekmurze]

Make sense! Thanks for the clear explanation 👍

[driesvints]

This change seems to have broken the package. The crawler class is still used here:

https://github.com/spatie/laravel-sitemap/blob/main/src/SitemapServiceProvider.php#L23

Without it, our daily scheduled task using SitemapGenerator::create started to failover. Could this be reverted?

[antonioribeiro]

I can check this later today, but I believe this change should be done on a major version as there might other implications and user needs.

I wasn't also able to run all the tests locally, as they need a testing server that I was not able to provide to Pest. Do we have the test requirements documented anywhere?

[freekmurze]

I'll revert this change and tag a new minor version.

@antonioribeiro Feel free to send a new PR where breaking changes are allowed. I'll release a new major for that. Also feel free to drop older Laravel and PHP versions and feel free to make use of new language / framework features 👍

[benbjurstrom]

@antonioribeiro appreciate you putting in the work on this. I reached a similar conclusion and have been running a lightweight fork with the auto-discovery functionality removed. But would would much rather see it added as option here. Let me know if you need any help with the updated PR.

[antonioribeiro]

Hey @benbjurstrom, if you can take the lead that would be great, I'm a bit swamped now. I did some testing and the error @driesvints was getting is just because the spatie/crawler was uninstalled because of the dependency removal, and needed to be reinstalled manually. But I'm not sure I can find time soon to jump and refresh the codebase as requested by Freek.