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

IBX-8335: Initial integration with API Platform #114

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

tischsoic
Copy link
Contributor

@tischsoic tischsoic commented Jul 2, 2024

Warning

Merge after Symfony 6 introduction in the DXP

🎫 Issue IBX-8335

Description:

Initial API Platform core integration with Language endpoint specification as an example.

Screenshot 2024-07-02 at 13 47 43

Installation

Because we have not moved from Symfony 5.4 to a higher version, fork of api-platform/core with dependencies downgraded to Symfony 5.4 (and related like psr etc) is needed.
This fork can be found here (branch downgraded-deps): https://github.com/tischsoic/core/tree/downgraded-deps
And diff of changes can be found here: tischsoic/core@69b4d35...tischsoic:core:downgraded-deps

My setup of the composer.json from the project root:

diff --git a/composer.json b/composer.json
index c3b4a42..ee8266c 100644
--- a/composer.json
+++ b/composer.json
@@ -8,6 +8,7 @@
         "ext-ctype": "*",
         "ext-iconv": "*",
         "ibexa/commerce": "5.0.x-dev",
+        "ibexa/rest": "dev-IBX-8335-full-api-platform",
         "symfony/console": "5.4.*",
         "symfony/dotenv": "5.4.*",
         "symfony/flex": "^1.17|^2",
@@ -31,6 +32,7 @@
         "optimize-autoloader": true,
         "preferred-install": {
             "ibexa/*": "source",
+            "api-platform/*": "source",
             "*": "dist"
         },
         "sort-packages": true
@@ -80,6 +82,10 @@
         "ibexa": {
             "type": "composer",
             "url": "https://updates.ibexa.co"
+        },
+        "api-platform/core": {
+            "type": "vcs",
+            "url": "https://github.com/tischsoic/core"
         }
     }
 }

Because of inconsistencies between. PHP version in different packages composer needs to ignore PHP version requirement:
composer update --ignore-platform-req=php

For QA:

Documentation:

@tischsoic tischsoic requested a review from a team July 2, 2024 12:21
@tischsoic tischsoic self-assigned this Jul 2, 2024
@tischsoic tischsoic force-pushed the IBX-8335-full-api-platform branch 2 times, most recently from ae550d9 to 3b250ed Compare July 3, 2024 09:58
@tischsoic tischsoic force-pushed the IBX-8335-full-api-platform branch from fb103f5 to 40cff86 Compare August 13, 2024 13:16
@tischsoic tischsoic force-pushed the IBX-8335-full-api-platform branch from b161278 to 50f6a84 Compare August 30, 2024 06:37
Copy link

sonarcloud bot commented Sep 4, 2024

Comment on lines +19 to +21
/**
* @var array<string>
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

We tend to use one-liner here.

/**
* @internal
*/
final class ClassNameResourceNameCollectionFactory implements ResourceNameCollectionFactoryInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be readonly?

Comment on lines +19 to +21
private readonly OpenApiFactoryInterface $decorated,
private readonly SchemasCollectionFactory $schemaCollectionFactory,
private readonly KernelInterface $kernel,
Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot the whole class be readonly instead?

}

$responses = $operation->getResponses();

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

* @var int $responseCode
* @var \ApiPlatform\OpenApi\Model\Response|array<string, array<mixed>> $response
*/
foreach ($operation->getResponses() as $responseCode => $response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
foreach ($operation->getResponses() as $responseCode => $response) {
foreach ($responses as $responseCode => $response) {

index_1: '@ibexa.api_platform.ibexa_openapi.factory'

ibexa.api_platform.ibexa_openapi.factory:
class: 'Ibexa\Bundle\Rest\ApiPlatform\OpenApiFactory'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class: 'Ibexa\Bundle\Rest\ApiPlatform\OpenApiFactory'
class: Ibexa\Bundle\Rest\ApiPlatform\OpenApiFactory

Comment on lines +13 to +14


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change



# Collecting schemas

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change


namespace Ibexa\Rest\ApiPlatform;

final class SchemasCollection implements \IteratorAggregate, \Countable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final class SchemasCollection implements \IteratorAggregate, \Countable
final readonly class SchemasCollection implements \IteratorAggregate, \Countable

use Symfony\Component\HttpKernel\KernelInterface;
use Symfony\Component\Yaml\Yaml;

final class SchemasProvider implements SchemasProviderInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Readonly class instead of all properties?

@konradoboza konradoboza requested a review from a team September 26, 2024 10:26
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.

4 participants