search

Sylius / SyliusAdminApiBundle

Administration REST API for Sylius eCommerce.
MIT License
11 stars 22 forks source link

arguments swapped #15

Closed dsbe-ak closed 2 years ago

dsbe-ak commented 2 years ago

Fix for "Required parameter $localeRepository follows optional parameter $validationGroups"

Zales0123 commented 2 years ago

Unfortunately, we cannot merge this change as it breaks the backward compatibility promise :( Thank you for you contribution though! 🖖

AndreasA commented 2 years ago

@Zales0123 What about one of the following changes, both should not be breaking:

1.) Change array $validationGroups = [], to array $validationGroups. The parameter is already required as the following parameter is required, so it isn't breaking. It is actually what the current deprecation message says. And overridding constructor parameters regarding types isn't an issue either.

2.) Change RepositoryInterface $localeRepository, to ?RepositoryInterface $localeRepository = null, and add Assert::notNull($localeRepository). That way would not be breaking either as the paramter has been required already, and overriden constructors won't have an issue, if the new signature makes it opitonal, neither will any service definitions. And with notNull it is still basically a required parameter.

I can create a PR or maybe @dsbe-ak would like to update this PR accordingly? Once I know which direction you want to go. Personally, I think the first is the better solutoin.

AndreasA commented 2 years ago

@Zales0123 Additionally, didn't this commit https://github.com/Sylius/SyliusAdminApiBundle/commit/076b2686ee07feddf4622c90518627fd9240a74c / PR https://github.com/Sylius/SyliusAdminApiBundle/pull/12 just change the OrderType in the same way as this PR?

As mentioned above if you want the change to be non breaking at all, both solutions above should work.