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

Extract sylius/admin-api-bundle out of Sylius Core (but keep it installed by default in existing minor releases) #12469

Merged
merged 5 commits into from
Apr 7, 2021

Conversation

pamil
Copy link
Contributor

@pamil pamil commented Mar 22, 2021

Why?

  • Two packages required by sylius/admin-api-bundle are blocking Sylius from upgrading to PHP 8.
  • We're rolling out v2 of the API, for both storefront and admin usage, which are getting more and more mature.
  • This bundle will be extracted as for Sylius v1.8 (so that we can still include bugfixes) and removed from the default installation in Sylius v1.10. However, it will be relatively simple to add it back to v1.10 if you're still using it - composer require and that's it.
  • Admin API is not as widely used as Shop API anyway.

Reference: #12298

@pamil pamil added RFC Discussions about potential changes or new features. Maintenance CI configurations, READMEs, releases, etc. PHP 8 labels Mar 22, 2021
@pamil pamil requested a review from a team as a code owner March 22, 2021 14:27
lchrusciel
lchrusciel previously approved these changes Mar 22, 2021
@pamil
Copy link
Contributor Author

pamil commented Mar 30, 2021

It almost works, but it requires adding COMPOSER_ROOT_VERSION=dev-master with composer update for any other branch that master (so 1.8 and 1.9 as well).

If not added, resolving dependencies fails with the following message:

  Problem 1
    - sylius/admin-api-bundle[v1.8.0, ..., v1.9.1] require sylius/core-bundle ^1.6 -> found sylius/core-bundle[v1.6.0, ..., v1.9.1] but these were not loaded, likely because it conflicts with another require.
    - Root composer.json requires sylius/admin-api-bundle ^1.8 -> satisfiable by sylius/admin-api-bundle[v1.8.0, ..., v1.9.1].

I'm looking for ideas to make it work by default without having to add this env variable; it hurts DX a lot.

@pamil
Copy link
Contributor Author

pamil commented Mar 30, 2021

Changing branch names helps, renaming 1.8 to 1.8.x fixes the problem, but I'm not sure how it would work out for new branches based on those. 1.8.x-anything still does not work.

@pamil
Copy link
Contributor Author

pamil commented Mar 30, 2021

Two more potential fixes:

  • explicitly specifying version in composer.json (prone to human error while tagging)
  • replacing packages for the whole minor version, like 1.8.*, instead of self.version (but then eg. sylius/sylius v1.8.15 could be installed even if sylius/core-bundle v1.8.20 is required and some dependency prevents from installing more recent version of sylius/sylius)

@pamil
Copy link
Contributor Author

pamil commented Mar 30, 2021

I guess I'll go with explicitly setting version in composer.json, we already do that in Kernel for Sylius version - it seems like the best possible solution.

@pamil pamil force-pushed the 1.8-extract-admin-api-bundle branch from a40674c to 5c2ba7b Compare March 30, 2021 14:36
@pamil pamil force-pushed the 1.8-extract-admin-api-bundle branch from c0b5253 to 856a0f4 Compare March 30, 2021 16:09
@pamil
Copy link
Contributor Author

pamil commented Mar 30, 2021

When upmerging, make sure all AdminApiBundle files are removed and the required version is bumped to ~1.9.0 for Sylius 1.9 and ^1.10 for Sylius 1.10 (dev-master).

The most important thing - make sure versions in composer.json are updated properly and are the same as the ones specified in the Kernel class from CoreBundle. Double-check before tagging any release.

@pamil pamil dismissed lchrusciel’s stale review March 30, 2021 18:55

Things have changed A LOT since that review

@pamil pamil requested a review from lchrusciel March 30, 2021 18:55
@pamil pamil merged commit 03546d5 into Sylius:1.8 Apr 7, 2021
@pamil pamil deleted the 1.8-extract-admin-api-bundle branch April 7, 2021 07:58
@@ -137,4 +137,35 @@ function (Options $options, $previousValue) use ($repository, $field, $criteria)
}
;
}

public static function getOneBy(RepositoryInterface $repository, string $field, array $criteria = []): \Closure
Copy link
Member

Choose a reason for hiding this comment

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

Where is it used?

GSadee added a commit that referenced this pull request Apr 12, 2021
This PR was merged into the 1.8 branch.

Discussion
----------

Follows changes made in #12469.

Tests are already included in the Admin API Bundle itself.

Commits
-------

9cd42af Remove AdminApiBundle-related tests
ab5a163 Fix Sylius package version in composer.json & require Sylius Admin API Bundle
lchrusciel added a commit that referenced this pull request Apr 20, 2021
…llation (pamil)

This PR was merged into the 1.10-dev branch.

Discussion
----------

See #12469.

`UPGRADE` instructions missing, I'll do them once we get this merged while customising Sylius-Standard to those changes.

Commits
-------

5f3d5a8 Remove AdminApiBundle from default Sylius installation
b53c6bc Replace incorrect typehints referencing AdminApiBundle
dbf8934 Fix serialization in AJAX after removal of AdminApiBundle
d54ab36 Load CoreBundle's serializer config only if AdminApiBundle is not loaded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance CI configurations, READMEs, releases, etc. RFC Discussions about potential changes or new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants