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

Make Cache dependency optional #1427

Merged
merged 13 commits into from
Jul 7, 2022
Merged

Conversation

Hanmac
Copy link
Contributor

@Hanmac Hanmac commented Jul 4, 2022

Subject

Changelog

### Changed
- Make Cache dependency optional

src/Admin/BaseBlockAdmin.php Outdated Show resolved Hide resolved
src/Admin/BaseBlockAdmin.php Outdated Show resolved Hide resolved
@@ -58,7 +58,7 @@ class PageAdmin extends AbstractAdmin
protected $siteManager;

/**
* @var CacheManagerInterface
* @var CacheManagerInterface|null
Copy link
Contributor

Choose a reason for hiding this comment

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

why it's possible to be null for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the plan is to remove it in next major version

Copy link
Contributor

Choose a reason for hiding this comment

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

then this bundle doesn't not require cache to works right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not anymore, that's why i moved it from require to require-dev for now

@@ -27,8 +27,6 @@
"doctrine/persistence": "^1.3.4 || ^2.0",
"sonata-project/admin-bundle": "^3.91.1",
"sonata-project/block-bundle": "^3.20",
"sonata-project/cache": "^2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

ohh is it possible to remove cache and cache-bundle in 3.x without any BC 😮

@eerison
Copy link
Contributor

eerison commented Jul 5, 2022

@Hanmac if it's not ready to review put as draft ;)

@Hanmac
Copy link
Contributor Author

Hanmac commented Jul 5, 2022

@Hanmac if it's not ready to review put as draft ;)

it should have been ready i think, just the lint stuff i couldn't get working in my IDE

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Although this seems ok, the cache is used as soon as it's installed.
But since SonataCacheBundle is installed by blockBundle, it's always used.

So we need to add a config option to disable the cache, instead of relying on isset($bundles['SonataCacheBundle']). And we'll be able to add a deprecation is the cache is not disabled.

@eerison
Copy link
Contributor

eerison commented Jul 5, 2022

Although this seems ok, the cache is used as soon as it's installed. But since SonataCacheBundle is installed by blockBundle, it's always used.

So we need to add a config option to disable the cache, instead of relying on isset($bundles['SonataCacheBundle']). And we'll be able to add a deprecation is the cache is not disabled.

hmmm then in case this option is "true", we will throw a deprecation message and remove cache completely in the next major?

@VincentLanglet
Copy link
Member

hmmm then in case this option is "true", we will throw a deprecation message and remove cache completely in the next major?

Depends if the option is enableCache or disableCache, but yes that's the idea.

cache: true should be the default value to not change any behavior, but trigger a deprecation.
cache: false will stop using the cache and send no deprecation.

4.x we remove the cache feature.

@Hanmac
Copy link
Contributor Author

Hanmac commented Jul 5, 2022

hmmm then in case this option is "true", we will throw a deprecation message and remove cache completely in the next major?

Depends if the option is enableCache or disableCache, but yes that's the idea.

cache: true should be the default value to not change any behavior, but trigger a deprecation. cache: false will stop using the cache and send no deprecation.

4.x we remove the cache feature.

should cache still be removed from the required or not yet?

@eerison
Copy link
Contributor

eerison commented Jul 5, 2022

hmmm then in case this option is "true", we will throw a deprecation message and remove cache completely in the next major?

Depends if the option is enableCache or disableCache, but yes that's the idea.

cache: true should be the default value to not change any behavior, but trigger a deprecation. cache: false will stop using the cache and send no deprecation.

4.x we remove the cache feature.

Ohhh nice, then I guess we do not need to Touch in this composer depencies, only in 4.x

@VincentLanglet
Copy link
Member

should cache still be removed from the required or not yet?

I just took a look at blockBundle and
https://github.com/sonata-project/SonataBlockBundle/blob/3.x/composer.json#L27
seems like cache was required but not CacheBundle.

The isset strategy could work then. WDYT @jordisala1991

VincentLanglet
VincentLanglet previously approved these changes Jul 5, 2022
@VincentLanglet
Copy link
Member

But if we move the dependency from require to require-dev, it also need to be in the conflict section to restrict the range of the versions

@VincentLanglet VincentLanglet requested a review from a team July 5, 2022 15:53
Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

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

Should we mark something to removal on 4.x? We can make on another PR if needed.

The idea of moving this to optional dependencies is to remove it on NEXT_MAJOR.

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Tests need to be fixed

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

You'll need to add @group legacy above the test

Issue1134Test::setUp from Sonata\PageBundle\Tests\Functional\Ticket

to avoid an error with the deprecation.

And we should also add a test with cache:false

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

The goal is to solve

Remaining self deprecation notices (1)
  1x: Using SonataCacheBundle is deprecated since sonata-project/page-bundle 3.27 and will be removed in 4.x
    1x in Issue1134Test::setUp from Sonata\PageBundle\Tests\Functional\Ticket

Maybe the group legacy should be added to the setUp of ResetableDBWebTestTest instead.

But you can also try to modify the test config
https://github.com/sonata-project/SonataPageBundle/blob/fed26c06856de8d6b1161d0f23001db7b8377e2b/tests/App/config/sonata.yaml
To add cache: false, instead.

@VincentLanglet
Copy link
Member

You don't need anymore the group legacy since you use cache: false

@VincentLanglet VincentLanglet requested review from jordisala1991 and a team July 6, 2022 16:05
@VincentLanglet VincentLanglet merged commit f2d5644 into sonata-project:3.x Jul 7, 2022
@VincentLanglet
Copy link
Member

Thanks

@eerison eerison mentioned this pull request Jul 7, 2022
@Hanmac Hanmac deleted the optionalCache branch July 10, 2022 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants