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

fix: document early hint package dependency #20507

Closed
wants to merge 1 commit into from

Conversation

94noni
Copy link
Contributor

@94noni 94noni commented Jan 3, 2025

avoid the internal error at runtime, lets document all requirements directly here, wdyt?

avoid the internal error at runtime, lets document all requirements directly here, wdyt?
@carsonbot carsonbot added this to the 6.4 milestone Jan 3, 2025
@@ -778,7 +778,7 @@ but they can be any type of resource.
.. note::

In order to work, the `SAPI`_ you're using must support this feature, like
`FrankenPHP`_.
`FrankenPHP`_ and have symfony/web-link as a dependency.
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't add this because the controller.rst file is mainly targeted at newcomers and this feels distracting.

Moreover, if you use the sendEarlyHints() shortcut but don't have web-link installed, you'll see an error message that is probably clear enough for newcomers too:

https://github.com/symfony/framework-bundle/blob/aaf86f38b483ce101c7e60be050bc0140431cfe2/Controller/AbstractController.php#L392-L396

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed I've seen the runtime error as described in the PR, it was just to document more
but I agree, this page is to generic for a doc
perhaps if I see other usage more precise related to http response and assets I will add it there
thank you !
closing meanwhile

@94noni 94noni closed this Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants