Problem/Motivation

Surfaced in #2483183-157: Make breadcrumb block cacheable.1 & #2483183-169: Make breadcrumb block cacheable.1.

According to @dawehner in #10:

Well the idea was to use the label in block UI, but yeah this is not longer possible, so yeah, IMHO we should drop the entire concept of the label, if you ask me. It is even a much smaller BC break than you think it is.

Proposed resolution

Remove the label from cache contexts.

Remaining tasks

Do it.

User interface changes

None.

API changes

Remove getLabel() from CacheContextInterface, remove the corresponding getLabels() from the CacheContextsManager.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal because it only removed unused code.
Disruption Slightly disruptive for contributed and custom modules because it will require a removing some code if a module defines it's own Cache Contexts.
CommentFileSizeAuthor
#17 make_all_cache_contexts-2550211-17.patch21.09 KBquietone
#11 make_all_cache_contexts-2550211-11.patch21.07 KBborisson_
#6 2550211-6.patch2.17 KBdawehner
#2 make_all_cache_contexts-2550211-2.patch15.33 KBborisson_

Issue fork drupal-2550211

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Wim Leers created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Active » Needs review
StatusFileSize
new make_all_cache_contexts-2550211-2.patch15.33 KB

Can't use the StringTranslationTrait because getLabel is a static method, and those can't use $this->t, a TranslationWrapper is possible though.

Attached a patch that does exactly this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Make all cache contexts use StringTranslationTrait instead of t() » Make all cache contexts use TranslationWrapper instead of t()

Sounds good.

Also: AAAHHH! Static method, that was the real reason! :) Thanks for pointing that out!

Pinged Gábor Hojtsy for review.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

Mhhhh

This is useful for using translation in very low level
* subsystems like entity definition and stream wrappers.

This doesn't really match the original usecase of the TranslationWrapper

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

That leaves us with no other options then? Won't fix? Or do you have other ideas?

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2550211-6.patch2.17 KB

Well, you could do something like this, realize that this issue is not a problem anyway (would you unit test that?) OR convert the method to not be a static method. This is just about admin UIs, so why do we care?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

If this stays static, we might as well continue calling t().

dawehner’s picture

dawehner
Primary language German
commented

@tim.plunkett What would you say if we convert it to a non static method?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Then we use StringTranslationTrait as usual, no? I don't know why it was static in the first place.

dawehner’s picture

dawehner
Primary language German
commented

Well the idea was to use the label in block UI, but yeah this is not longer possible, so yeah, IMHO we should drop the entire concept of the label, if you ask me. It is even a much smaller BC break than you think it is.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new make_all_cache_contexts-2550211-11.patch21.07 KB

Attached patch removes getLabel from the CacheContextInterface interface and getLabels from the CacheContextsManager. It also removes the implementations and fixes the CacheContextsManagerTest (that used the getLabels). I ran phpunit and that passes, let's see if the integration tests need changes.

dawehner’s picture

dawehner
Primary language German
commented

Its an API change but I think it is the right one.

We need a beta evaluation to justify that change.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Title: Make all cache contexts use TranslationWrapper instead of t() » Remove the getLabel method from Cache Contexts
Priority: Minor » Normal
Issue summary: View changes

Changed IS

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Well the idea was to use the label in block UI, but yeah this is not longer possible, so yeah, IMHO we should drop the entire concept of the label, if you ask me.

This also means contrib is prevented from building certain advanced modules, which allow you to define or modify the cacheability of things built entirely in the UI.

But that is indeed the only reason.

I personally am +1 for this, we could still add a LabeledCacheContextInterface in the future that cache contexts can choose to opt in to.

I think we need catch and Fabian's +1s too.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

quietone commented
StatusFileSize
new make_all_cache_contexts-2550211-17.patch21.09 KB

Reroll.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
@@ -60,29 +60,6 @@ public function getAll() {
-  public function getLabels($include_calculated_cache_contexts = FALSE) {

From a BC layer POV maybe keeping this method could be helpful. It could be converted to return an empty array, so existing code doesn't fail.

quietone’s picture

quietone commented
Issue tags: -Novice, -php-novice

Since there is ongoing discussion of what the task actually is, removing the novice and php-novice tag.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

I would simply add a @deprecated on the interface, so we can remove it in 9.x, there's very little benefit to touch the code to return nothing and then remove it later IMHO.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +stale-issue-cleanup

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Postponed (maintainer needs more info) » Needs work

This is still a useful thing to do, we're not using these. However, we now have a deprecation policy that we should follow for this.

\Drupal\Core\Cache\Context\CacheContextsManager::getLabels should probably also be deprecated?

meeni_dhobale made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.