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.

Sign up for GitHub

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

Jump to bottom

Remove Cache #1430

Merged
merged 1 commit into from
Jul 8, 2022
Merged

Remove Cache #1430

merged 1 commit into from
Jul 8, 2022

Conversation

Hanmac
Copy link
Contributor

@Hanmac Hanmac commented Jul 7, 2022 •
edited by jordisala1991
Loading

Subject

I am targeting this branch, because it is BC break.

Changelog

### Removed
- Remove cache related code.

To do

  • Update the tests;
  • Update the documentation;
  • Add an upgrade note.

@Hanmac Hanmac mentioned this pull request Jul 7, 2022
Closed
@eerison
Copy link
Contributor

eerison commented Jul 7, 2022

@Hanmac I guess you need to rebase your branch

@Hanmac
Copy link
Contributor Author

Hanmac commented Jul 7, 2022

@Hanmac I guess you need to rebase your branch

i think i need to wait for this until the CI has merged 3.x into 4.x

@eerison
Copy link
Contributor

eerison commented Jul 7, 2022

@Hanmac I guess you need to rebase your branch

i think i need to wait for this until the CI has merged 3.x into 4.x

I thought that Vincent has merged it, my bad

@jordisala1991
Copy link
Member

Now it is merged, please rebase.

@SonataCI
Copy link
Collaborator

SonataCI commented Jul 7, 2022

Could you please rebase your PR and fix merge conflicts?

jordisala1991
jordisala1991 previously approved these changes Jul 7, 2022
View reviewed changes
VincentLanglet
VincentLanglet requested changes Jul 7, 2022
View reviewed changes
docs/reference/advanced_configuration.rst Outdated Show resolved Hide resolved
docs/reference/advanced_usage.rst Outdated Show resolved Hide resolved
docs/reference/installation.rst Outdated Show resolved Hide resolved
src/Cache/BlockEsiCache.php Show resolved Hide resolved
src/DependencyInjection/Configuration.php Show resolved Hide resolved
src/DependencyInjection/Configuration.php
@@ -312,33 +291,6 @@ public function getConfigTreeBuilder()
->end()
->end()

->arrayNode('caches')
Copy link
Member

Choose a reason for hiding this comment

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

We need to trigger a deprecation for this node on 3.x

src/DependencyInjection/Configuration.php
@@ -365,9 +317,6 @@ public function getConfigTreeBuilder()
->booleanNode('direct_publication')
->info($directPublicationInfo)
->defaultValue(false)
->end()
->booleanNode('cache')
Copy link
Member

Choose a reason for hiding this comment

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

I assume we have to keep this option for now in to order to avoid a BC break without any deprecation.

The default value should be change to false to be consistent and the option will not be used and removed only on 5.x.
On 4.x it will trigger a deprecation when set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could deprecate it in 3x?

tests/App/config/sonata.yaml Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Member

@jordisala1991, just saw you approved.
Should we deprecate the class/config on 3.x as always ; or do we take a shortcut here ?

@jordisala1991
Copy link
Member

Not sure if we need upgrade note, since everything was deprecated and we have a default note for deprecated code. Wdyt @VincentLanglet ?

@VincentLanglet
Copy link
Member

VincentLanglet commented Jul 7, 2022 •
edited
Loading

Not sure if we need upgrade note, since everything was deprecated and we have a default note for deprecated code. Wdyt @VincentLanglet ?

We just didn't directly deprecated the removed class/config.

For the config, you can still use

cache: false

and others cache config without any warning.

For the classes, if someone is using BlockEsiCache or others classes, directly in his code, he'll get some surprise in 4.x.

I would say it doesn't cost us too much to add

@deprecated since sonata-project/page-bundle version 3.x and will be removed in 4.0.

on classes, and call setDeprecated (like https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/DependencyInjection/Configuration.php#L166-L171) on config keys.

Do you mind doing a PR on 3.x @Hanmac ?

@Hanmac
Copy link
Contributor Author

Hanmac commented Jul 8, 2022

Not sure if we need upgrade note, since everything was deprecated and we have a default note for deprecated code. Wdyt @VincentLanglet ?

We just didn't directly deprecated the removed class/config.

For the config, you can still use

cache: false

and others cache config without any warning.

For the classes, if someone is using BlockEsiCache or others classes, directly in his code, he'll get some surprise in 4.x.

I would say it doesn't cost us too much to add

@deprecated since sonata-project/page-bundle version 3.x and will be removed in 4.0.

on classes, and call setDeprecated (like https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/DependencyInjection/Configuration.php#L166-L171) on config keys.

Do you mind doing a PR on 3.x @Hanmac ?

Deprecated the Cache classes, and the config options:
#1435

@VincentLanglet
Copy link
Member

Thanks, I merged 3.x into 4.x so there is just some conflicts to solve

@SonataCI
Copy link
Collaborator

SonataCI commented Jul 8, 2022

Could you please rebase your PR and fix merge conflicts?

@VincentLanglet VincentLanglet merged commit 0cc5865 into sonata-project:4.x Jul 8, 2022
@Hanmac Hanmac deleted the removeCacheMajor branch July 8, 2022 08:08
treehousedevelopers pushed a commit to treehousedevelopers/SonataPageBundle that referenced this pull request Jul 13, 2022
@Hanmac @marekryk
remove Cache and CacheBundle (sonata-project#1430)
7c9f665 
# Conflicts:
#	composer.json
#	src/Admin/BaseBlockAdmin.php
#	src/Admin/PageAdmin.php
#	src/Admin/SnapshotAdmin.php
#	src/Cache/BlockEsiCache.php
#	src/Cache/BlockJsCache.php
#	src/Cache/BlockSsiCache.php
#	src/DependencyInjection/Configuration.php
#	src/DependencyInjection/SonataPageExtension.php
#	src/Resources/config/admin.xml
#	src/SonataPageBundle.php
#	tests/App/AppKernel.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VincentLanglet VincentLanglet VincentLanglet approved these changes

@jordisala1991 jordisala1991 jordisala1991 left review comments

Assignees
No one assigned
Labels
major
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Hanmac @eerison @jordisala1991 @SonataCI @VincentLanglet