Skip to content

Symfony 6.3 and API Platform 2.7 #165

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

Merged
merged 55 commits into from
Nov 9, 2023
Merged

Symfony 6.3 and API Platform 2.7 #165

merged 55 commits into from
Nov 9, 2023

Conversation

rimi-itk
Copy link
Contributor

@rimi-itk rimi-itk commented Sep 26, 2023

Link to ticket

https://jira.itkdev.dk/browse/DISPLAY-1030

Description

Updates to Symfony 6.3 and API Platform 2.7 (cf. https://api-platform.com/docs/core/upgrade-guide/#im-migrating-from-26-and-want-to-prepare-for-30)

Checklist

  • My code is covered by test cases.
  • My code passes our test (all our tests).
  • My code passes our static analysis suite.
  • My code passes our continuous integration process.

The API Specification has some (hopefully) insignificant changes:

  • Empty descriptions added
  • Generic name "Resource" changed to actual resource name in descriptions, e.g. "ScreenGroupCampaign identifier" vs. "Resource identifier"
  • links are not set in response examples (do we use these links?) This is related to the line https://github.com/api-platform/core/blob/v2.6.8/src/OpenApi/Factory/OpenApiFactory.php#L188 (and others) having been removed and refactored extensively.
  • examples and additional properties added in schema

https://gist.github.com/rimi-itk/ff2c9400aa8d6d76caae698053b54638#file-compare-api-specs-sh has been used to test the API spec piece by piece and compare it with https://github.com/os2display/display-api-service/tree/develop

@rimi-itk rimi-itk changed the base branch from develop to feature/DISPLAY-1007-cleanup-draft-without-entity-changes October 3, 2023 12:12
@rimi-itk rimi-itk marked this pull request as ready for review November 6, 2023 13:36
Copy link
Contributor

@tuj tuj left a comment

Choose a reason for hiding this comment

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

Generally looks good.

- "traefik.http.routers.${COMPOSE_PROJECT_NAME}Mailhog.rule=Host(`mailhog-${COMPOSE_DOMAIN}`)"
- "traefik.http.services.${COMPOSE_PROJECT_NAME}Mailhog.loadbalancer.server.port=8025"
- "traefik.http.routers.${COMPOSE_PROJECT_NAME}mail.rule=Host(`mail-${COMPOSE_DOMAIN}`)"
- "traefik.http.services.${COMPOSE_PROJECT_NAME}mail.loadbalancer.server.port=8025"

redis:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move redis and node to docker-compose.override.yml ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be the ITK Dev way to do it, but 04cbc36

@@ -0,0 +1,39 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it should be removed.
Why was it added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added in 30ca376 to make our “Validate Doctrine Schema” stop complaining. It sure is weird …

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A comment on this weird stuff added in d453c11

@@ -67,6 +59,7 @@ public function normalize($object, $format = null, array $context = []): array|s
$data = ['@context' => '/contexts/'.$context['output']['name']];

if (isset($context['operation_type']) && OperationType::SUBRESOURCE === $context['operation_type']) {
// FIXME: When do we end up here? `getSubresourceIriFromResourceClass` is not defined anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed?

{
return true;
// FIXME: Figure out what to actually return here?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this handled?

…raft-without-entity-changes' into feature/DISPLAY-1030-symfony-update
@rimi-itk rimi-itk requested a review from tuj November 8, 2023 15:11
@tuj tuj deleted the branch os2display:develop November 9, 2023 10:05
@tuj tuj closed this Nov 9, 2023
@tuj tuj reopened this Nov 9, 2023
@rimi-itk rimi-itk changed the base branch from feature/DISPLAY-1007-cleanup-draft-without-entity-changes to develop November 9, 2023 10:18
@rimi-itk rimi-itk changed the title Symfony update Symfony 6.3 and API Platform 2.7 Nov 9, 2023
@tuj tuj merged commit a549187 into os2display:develop Nov 9, 2023
@rimi-itk rimi-itk deleted the feature/DISPLAY-1030-symfony-update branch November 9, 2023 12:37
turegjorup added a commit to aroskanalen/display-api-service that referenced this pull request Feb 6, 2024
turegjorup added a commit to aroskanalen/display-api-service that referenced this pull request Feb 6, 2024
turegjorup added a commit to aroskanalen/display-api-service that referenced this pull request Feb 6, 2024
turegjorup added a commit to aroskanalen/display-api-service that referenced this pull request Feb 8, 2024
turegjorup added a commit to aroskanalen/display-api-service that referenced this pull request Feb 8, 2024
turegjorup added a commit to aroskanalen/display-api-service that referenced this pull request Feb 8, 2024
tuj added a commit that referenced this pull request Feb 23, 2024
…odified_bug

#165: Bugfix for "relations modified" not set correctly on OneToMany relations
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.

2 participants