Init resource class list factory with attributes#986
Conversation
loic425
force-pushed
the
feature/resource-name-factory
branch
from
cb727cd to
59ffdae
Compare
loic425
force-pushed
the
feature/resource-name-factory
branch
from
59ffdae to
beb7ae6
Compare
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Sylius\Resource\Metadata\Resource\Factory; |
There was a problem hiding this comment.
I think it will be better like this
| namespace Sylius\Resource\Metadata\Resource\Factory; | |
| namespace Sylius\Resource\Metadata\Resource\NameCollection\Factory; |
WDYT @diimpp?
There was a problem hiding this comment.
But in this, case we should move The ResourceNameCollection too, and moved all the service related to MetadataCollection factories in another namespace too.
I have an issue, cause I forgot some "Experimental" tags and then we need class and interface aliases for that. So maybe we could keep this coherent.
There was a problem hiding this comment.
Discussed in slack; Since NameCollection is just a VO, then extensive namespacing is not needed and current one looks good enough.
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Sylius\Resource\Metadata\Resource\Factory; |
There was a problem hiding this comment.
Discussed in slack; Since NameCollection is just a VO, then extensive namespacing is not needed and current one looks good enough.
| <service id="sylius_resource.metadata.resource.name_collection.factory" alias="sylius_resource.metadata.resource.name_collection.factory.attributes" /> | ||
| <service id="Sylius\Resource\Metadata\Resource\Factory\ResourceNameCollectionFactoryInterface" alias="sylius_resource.metadata.resource.name_collection.factory" /> | ||
|
|
||
| <service id="sylius_resource.metadata.resource.name_collection.factory.attributes" |
There was a problem hiding this comment.
Naming should match existing IDs.
Example
sylius.resource_metadata_collection.factory.attributes| <service id="sylius_resource.metadata.resource.name_collection.factory.attributes" | |
| <service id="sylius.resource_name_collection.factory.attributes" |
There was a problem hiding this comment.
So you prefer we change all the service ids in 2.0 than start using the new naming for the new service ids?
There was a problem hiding this comment.
@loic425 yes, but it's more about consistency -- two files in same directory shouldn't have different ID naming scheme.
So I guess sylius_resource.*** can be left for 2.0.
loic425
force-pushed
the
feature/resource-name-factory
branch
2 times, most recently
from
9258717 to
a0e8aac
Compare
loic425
force-pushed
the
feature/resource-name-factory
branch
from
a0e8aac to
a866510
Compare
|
Interesting discussions there But we are in a specific branch and also everything are tagged as experimental here, so we can adjust later. |
| final class ResourceClassList implements \IteratorAggregate, \Countable | ||
| { | ||
| /** | ||
| * @param string[] $names |
There was a problem hiding this comment.
--- public function __construct(private readonly array $names = [])
+++ public function __construct(private readonly array $classNames = [])
+++ public function __construct(private readonly array $classes = [])There was a problem hiding this comment.
src/Component/src/Metadata/Resource/Factory/AttributesResourceClassListFactory.php uses $classes internally, so I guess it should be preferred.
diimpp
changed the title
loic425
force-pushed
the
feature/resource-name-factory
branch
2 times, most recently
from
cbffe74 to
cfffa21
Compare
Co-authored-by: Dmitri Perunov <diimpp@gmail.com>
loic425
force-pushed
the
feature/resource-name-factory
branch
from
cfffa21 to
7bc5c10
Compare
| Q | A | --------------- | ----- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | | License | MIT This work have ben reviewed via multiple PRs. #985 #986 #987 It reworks the routing system to allow more routing system than the attributes one in the near future.
| Q | A | --------------- | ----- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | | License | MIT This work have ben reviewed via multiple PRs. Sylius/SyliusResourceBundle#985 Sylius/SyliusResourceBundle#986 Sylius/SyliusResourceBundle#987 It reworks the routing system to allow more routing system than the attributes one in the near future.
2 participants
Based on #985