Remove core's parameter typehints of CacheableMetadata within interfaces and base classes

Mh, so \Drupal\Core\Menu\MenuParentFormSelector::parentSelectOptionsTreeWalk calls out to

$cacheability
          ->merge

which is not on the interface, is this really what we want?

In generla having a BC layer aka. an old instance of the class seems totally worth it, and doesn't cause issues for people, so I don't get why breaking interfaces here is okay.

#2: Because we should typehint to interfaces, not implementations.

@Wim Leers
Sure, but to be honest CacheableMetadata is a value object for me, so additional implementations don't make that much sense.

Note that callers of this code won't have to change anything. Only implementations (of which likely zero exist at this time) would have to be updated.

Also, once those two are removed, we'll have zero typehints to CacheableMetadata left! The only remaining ones then are in test coverage/ internals.

This is again shows that typehinting to those concrete classes was simply a mistake, an oversight, a bug. This fixes that. Everything else typehints to (Refinable)CacheableDependencyInterface. For a very similar example, see \Drupal\Core\Menu\LocalTaskManagerInterface::getTasksBuild().

I still don't see why we freak out ..., for the static method and other reasons we need to BC when we rename anyway.

Only implementations (of which likely zero exist at this time) would have to be updated.

You know, we promised the BC. I think we should break if just if there is an actual benefit.

This is again shows that typehinting to those concrete classes was simply a mistake, an oversight, a bug

I don't disagree with that, but that is simply not my point here.

Oh yeah both directions are just horrible. What about using https://secure.php.net/manual/en/function.class-alias.php ? (see https://3v4l.org/Ntu7n it runs perfectly)

<?php

class A {}

class_alias('A', 'B');

function f1(A $a) {
    
}

function f2(B $b) {
    
}

$a = new A();
f1($a);
f2($a);

$b = new B();
f1($b);
f2($b);

In general renaming classes feels really painfull.

Woah! Mind=blown.

@dawehner++
@dawehner++

So, then would you agree with NOT doing this issue, and doing this in the parent issue (#2561773: CacheableMetadata is misnamed)?

1. Rename CacheableMetadata to Cacheability
2. Create an alias for BC.
3. Mark merge() as deprecated, to be removed in 8.2 (edited)

SO yeah for the alias you afaik put a class_alias() call into the file which would have contained the former class.

@Wim Leers
I'm confused about your point 3, there are like a gazillion amount of calls to merge()

Too late now. Moving to 8.2, removing RC target triage tag.

However, I still think the patch in #7 makes sense, even independently of #2561773: CacheableMetadata is misnamed. Per the first paragraph of #10 that is okay.

Comments #10-paragraph-2, #11, #12, #13 and #14 actually belong on #2561773: CacheableMetadata is misnamed.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Wondering after 7 years for Drupal10.1 if this is still relevant?

I think between changing the type hints to the interface and using class_alias() that would allow us to do the other issue without breaking bc, so still seems valid.

Comments on the patch:

1. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelectorInterface.php
   @@ -26,7 +26,7 @@
       *   the values are a menu name or link title indented by depth.
       */
   -  public function getParentSelectOptions($id = '', array $menus = NULL, CacheableMetadata &$cacheability = NULL);
   +  public function getParentSelectOptions($id = '', array $menus = NULL, RefinableCacheableDependencyInterface $cacheability = NULL);
    
      /**
   

   I doubt anyone is subclassing this, but I think we should probably remove the entire parameter from the interface in Drupal 10, add a commented out one to indicate it's going to be added back with the new type hint, then change the type hint in Drupal 12. Pretty sure this allows an implementation to update their own type hint in Drupal 10, see example 3 on #3050720: [Meta] Implement strict typing in existing code.

2. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -283,10 +284,10 @@ protected function prepareViewResult(array $result) {
       */
   -  public function alterCacheMetadata(CacheableMetadata $cache_metadata) {
   +  public function alterCacheMetadata(RefinableCacheableDependencyInterface $cache_metadata) {
      }
   

   And the same here.