The warmup part ofcache:clear does not work well, and does not deliver the guarantee that the generated cache is exactly the same as the one that would have been generated viacache:warmup.

As one of the goal of Symfony 4 is to be able to generate all the cache for read-only filsystem, I propose to deprecate the warmup part ofcache:clear in 3.3 to be able to remove it completely in 4.0. In 4.0, the--no-warmup option would be a noop (and can then be removed in 5.0).

carsonbot added Status: Needs Review FrameworkBundle Deprecation labels

Dec 23, 2016

xabbuh reviewed

Dec 23, 2016

View reviewed changes

UPGRADE-3.3.md Outdated

		FrameworkBundle
		---------------

		* The cache:clear command should always be called the --no-warmup option.

Copy link

Member

xabbuhDec 23, 2016

Choose a reason for hiding this comment

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

missing "without"

Copy link

Member

xabbuhDec 23, 2016

Choose a reason for hiding this comment

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

Sorry, I meant "with".

Copy link

MemberAuthor

fabpotDec 23, 2016

Choose a reason for hiding this comment

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

fixed

src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php Outdated

		}
		$filesystem->remove($warmupDir);
		}
		@trigger_error('Calling cache:clear without the --no-warmup option is deprecated version 3.3. Cache warmup should be done with the cache:warmup command instead.',E_USER_DEPRECATED);

Copy link

Member

xabbuhDec 23, 2016

Choose a reason for hiding this comment

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

missing "since"

Copy link

MemberAuthor

fabpotDec 23, 2016

Choose a reason for hiding this comment

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

fixed

fabpot force-pushed thecache-clear-warmup-deprecation branch fromdffd2fc tod0b43d8Compare

December 23, 2016 21:16

Copy link

Contributor

ro0NL commentedDec 24, 2016•
edited
Loading

@fabpot is the deprecation notice really needed though? I mean it can be a info/warning log message on behavior change as well right? And perhaps dont log anything in non-quiet mode, but display it to the user instead. Meaning we could deprecate--no-warmup right away..

Point is; half the internet doesbin/console c:c by now, and im not sure getting a deprecationby default, makes really sense 🤔 (opposed to informing aboutcache:warmup / changing behavior).

Copy link

Member

stof commentedDec 24, 2016

This looks like a good idea to me

Copy link

MemberAuthor

fabpot commentedDec 24, 2016

@stof you think@ro0NL is a good idea? or just the fact that we deprecate the warmup process in cache:clear?

Copy link

Member

stof commentedDec 24, 2016

I was talking about the deprecation of the warmup part. But@ro0NL is right about deprecations by default though

Copy link

Contributor

ro0NL commentedDec 24, 2016•
edited
Loading

I know for sure i wouldnt care about the deprecation; it would work rather confusing instead, ie;

I must/should run cache:warmup now to upgrade properly?

While we're actually trying to separate things, and let users decide. Hence, it should be informational imo.

Meaning we could deprecate --no-warmup right away..

Not sure though, as removing it is not the right upgrade path till 4.0.. tough one :)

edit: yep.. we should definitely deprecate--no-warmupafter the default behavior has changed.

Copy link

Contributor

ro0NL commentedDec 24, 2016•
edited
Loading

Now im not even sure we should log/display anything opposed to adding a changelog note?

Doing it in code kinda implies "people dont read changelogs" ;-)

We couldjust drop it, right?

nicolas-grekas added this to the3.3 milestone

Dec 26, 2016

fabpot force-pushed thecache-clear-warmup-deprecation branch 2 times, most recently fromd565d18 tod8b0c2cCompare

February 16, 2017 23:59

fabpot force-pushed thecache-clear-warmup-deprecation branch fromd8b0c2c toe9533f0Compare

March 21, 2017 21:59

Copy link

MemberAuthor

fabpot commentedMar 21, 2017

Just dropping the flag won't work as it would break people using it currently. I don't see any other better way.

[FrameworkBundle] deprecated cache:clear with warmup

7ed3237

fabpot force-pushed thecache-clear-warmup-deprecation branch frome9533f0 to7ed3237Compare

March 21, 2017 22:16

Copy link

MemberAuthor

fabpot commentedMar 22, 2017

I'm going to merge it as is. We will have 2 months to tweak the message if needed.

fabpot merged commit7ed3237 intosymfony:master

Mar 22, 2017

fabpot added a commit that referenced this pull request

Mar 22, 2017

feature#21038[FrameworkBundle] deprecated cache:clear with warmup (…

3495b35

…fabpot)This PR was merged into the 3.3-dev branch.Discussion----------[FrameworkBundle] deprecated cache:clear with warmup| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aThe warmup part of `cache:clear` does not work well, and does not deliver the guarantee that the generated cache is exactly the same as the one that would have been generated via `cache:warmup`.As one of the goal of Symfony 4 is to be able to generate all the cache for read-only filsystem, I propose to deprecate the warmup part of `cache:clear` in 3.3 to be able to remove it completely in 4.0. In 4.0, the `--no-warmup` option would be a noop (and can then be removed in 5.0).Commits-------7ed3237 [FrameworkBundle] deprecated cache:clear with warmup

javiereguiluz mentioned this pull request

Mar 26, 2017

[FrameworkBundle] deprecated cache:clear with warmupsymfony/symfony-docs#7696

Closed

fabpot deleted the cache-clear-warmup-deprecation branch

April 7, 2017 17:17

fabpot mentioned this pull request

May 1, 2017

Release v3.3.0-BETA1#22603

Merged

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request

Aug 8, 2017

Revert "featuresymfony#21038[FrameworkBundle] deprecated cache:clea…

34369ad

…r with warmup (fabpot)"This reverts commit3495b35, reversingchanges made to7f7b897.

nicolas-grekas mentioned this pull request

Aug 8, 2017

Revert "feature #21038 [FrameworkBundle] deprecated cache:clear with warmup (fabpot)"#23825

Merged

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request

Aug 8, 2017

Revert "featuresymfony#21038[FrameworkBundle] deprecated cache:clea…

9009970

…r with warmup (fabpot)"This reverts commit3495b35, reversingchanges made to7f7b897.

fabpot added a commit that referenced this pull request

Aug 18, 2017

bug#23825Revert "feature#21038[FrameworkBundle] deprecated cache:…

4fdaa3a

…clear with warmup (fabpot)" (nicolas-grekas)This PR was merged into the 3.3 branch.Discussion----------Revert "feature#21038 [FrameworkBundle] deprecated cache:clear with warmup (fabpot)"This reverts commit3495b35, reversingchanges made to7f7b897.| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#21038| License       | MIT| Doc PR        | -Sibling to#23792: there is no need to trigger the deprecation in 3.3 if we un-deprecate in 3.4.Commits-------9009970 Revert "feature#21038 [FrameworkBundle] deprecated cache:clear with warmup (fabpot)"

nicolas-grekas added a commit that referenced this pull request

Aug 18, 2017

Merge branch '3.3' into 3.4

f751ac9

* 3.3:  Revert "feature#21038 [FrameworkBundle] deprecated cache:clear with warmup (fabpot)"

nicolas-grekas added a commit that referenced this pull request

Aug 18, 2017

Merge branch '3.4'

c8d7a6f

* 3.4:  [HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup  [DI] Fix merging of env vars in configs  Allow to get alternatives when ServiceNotFoundException occurs.  [DI] Rererence parameter arrays when possible  Revert "feature#21038 [FrameworkBundle] deprecated cache:clear with warmup (fabpot)"

fabpot mentioned this pull request

Aug 28, 2017

Release v3.3.7#24013

Merged