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(hydra): store and use hydra context in a local variable #6765

Merged

Conversation

valentin-dassonville
Copy link
Contributor

Q A
Branch 4.0
License MIT

@valentin-dassonville valentin-dassonville force-pushed the fix/hydra-context-static-asset branch 2 times, most recently from 97db06a to 0454297 Compare October 30, 2024 13:34
@vincentchalamon
Copy link
Contributor

@soyuka it seems to work on api-platform/demo#472

@dunglas
Copy link
Member

dunglas commented Nov 4, 2024

Can't we merge the Hydra context in the context we generate? So we avoid an extra HTTP request.

@valentin-dassonville valentin-dassonville force-pushed the fix/hydra-context-static-asset branch from 0a4dedf to 92009ff Compare November 5, 2024 13:42

Verified

This commit was signed with the committer’s verified signature.
vinceAmstoutz Vincent Amstoutz
@valentin-dassonville valentin-dassonville force-pushed the fix/hydra-context-static-asset branch from 92009ff to 36e4d08 Compare November 5, 2024 13:51
@valentin-dassonville valentin-dassonville changed the title fix(hydra): store and use hydra context as static asset fix(hydra): store and use hydra context in a local variable Nov 5, 2024
And the JSON node "@context[0]" should be equal to:
"""
{
"@context": {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest doing something dynamic like:

  1. Download the original context from Hydra's website
  2. Assert that the context we generate is in sync with the upstream one

This will have the benefit of not bloating the tests and making the CI red if upstream changes the context.

WDYT?

@@ -53,4 +52,905 @@ public function getResourceContext(string $resourceClass, int $referenceType = U
* Gets the URI of the given resource context.
*/
public function getResourceContextUri(string $resourceClass, int $referenceType = UrlGeneratorInterface::ABS_PATH): string;

public const HYDRA_CONTEXT = [
'@context' => [
Copy link
Member

Choose a reason for hiding this comment

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

Maybe could we store this in an external PHP file that we automatically generate from the upstream JSON (so we can regenerate the file if needed).

@soyuka soyuka force-pushed the fix/hydra-context-static-asset branch from ade6748 to d24d2ce Compare November 6, 2024 09:33

Verified

This commit was signed with the committer’s verified signature.
vinceAmstoutz Vincent Amstoutz
@soyuka soyuka force-pushed the fix/hydra-context-static-asset branch from d24d2ce to a7e14dc Compare November 6, 2024 09:38
soyuka and others added 2 commits November 6, 2024 11:34

Verified

This commit was signed with the committer’s verified signature.
vinceAmstoutz Vincent Amstoutz
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@soyuka soyuka merged commit 9ac3661 into api-platform:4.0 Nov 6, 2024
59 checks passed
@soyuka
Copy link
Member

soyuka commented Nov 6, 2024

Thanks @valentin-dassonville !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants