Skip to content

Conversation

@loic425
Copy link
Member

@loic425 loic425 commented Mar 14, 2025

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets #1029
License MIT

Same feature as I did on api-platform/core#7017

@loic425 loic425 marked this pull request as draft March 14, 2025 10:05
@loic425 loic425 force-pushed the feat/php-file-resource-extractor branch from 0316f8a to 306b9b4 Compare June 3, 2025 08:14
@loic425 loic425 marked this pull request as ready for review June 3, 2025 08:17
$property->setValue($resource, $resolvedValue);
}

$this->resources = [$resource];
Copy link
Member

Choose a reason for hiding this comment

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

Function public function getResources(): array is meant to extract multiple resources by multiple paths, but this line overrides $resources.

Is this planned behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, this is really strange. I need to check that ASAP and on APIP too.

Copy link
Member Author

@loic425 loic425 Jun 3, 2025

Choose a reason for hiding this comment

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

AHAH, ok, I remember now, the path is a specific file (not a directory). So we have only one resource metadata per file. That's not the case we some other resource extractor on APIP where we can declare serveral resources in the same file.

Copy link
Member Author

Choose a reason for hiding this comment

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

we configure a directory on the config, but the Symfony Finder will extract files.

Copy link
Member Author

@loic425 loic425 Jun 3, 2025

Choose a reason for hiding this comment

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

Copy link
Member

@diimpp diimpp Jun 3, 2025

Choose a reason for hiding this comment

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

Right now if you have multiple file paths, each extract will override previous extract, so even if you have 5 different paths, you only will have single resource as output.

This change will fix that.

        $this->resources = [];
        foreach ($this->paths as $path) {
---         $this->extractFromPath($path);
       }
        $this->resources = [];
        foreach ($this->paths as $path) {
+++        $this->resources[] = $this->extractFromPath($path);
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix, thx a lot.

@loic425 loic425 force-pushed the feat/php-file-resource-extractor branch from 306b9b4 to b94661e Compare June 3, 2025 12:40
@loic425 loic425 force-pushed the feat/php-file-resource-extractor branch from b94661e to bf8ae48 Compare June 3, 2025 12:44
Comment on lines +38 to +40
/**
* @inheritdoc
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* @inheritdoc
*/

@loic425 loic425 force-pushed the feat/php-file-resource-extractor branch from bf84e79 to 910820f Compare June 4, 2025 07:14
@loic425 loic425 changed the base branch from 1.13 to 1.14 June 4, 2025 10:27
@loic425 loic425 changed the title Add PHP file resource extractor [Metadata] Add PHP file resource extractor Jun 4, 2025
@loic425
Copy link
Member Author

loic425 commented Jun 4, 2025

Test with the Skeleton are not working here but works in next PR :( I don't understand why, it works on my local 🥇

@loic425 loic425 changed the base branch from 1.14 to external-routing-files June 4, 2025 16:15
@loic425 loic425 merged commit f95a255 into Sylius:external-routing-files Jun 4, 2025
33 of 39 checks passed
@loic425 loic425 deleted the feat/php-file-resource-extractor branch June 5, 2025 10:04
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