Fix null $cid in CacheCollector classes

Guessing the error in the pipeline is the fact a NULL is still passed and end up at mb_check_encoding

Indeed, the $cid was reset to NULL and the check for $this->cid === '' no longer evaluated to TRUE.

I considered empty instead of === '' and although that might catcher extending code setting null, this also makes the check check against falsy which is not ideal. I think this change is all good.

Seems straightforward enough for me to sign off on this. Thanks!

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch → closed merge request !9396

The MenuActiveTrail change causes a regression for us. I am looking into the immediate cause on our end.

This breaks the Menu Trail By Path module, which extends that class.

Looks like there are a few more contrib modules that might break here:

• https://git.drupalcode.org/project/entity_manager/-/blob/1.0.x/src/Menu/...
• https://git.drupalcode.org/project/gin_toolbar/-/blob/8.x-1.x/src/Menu/G...
• https://git.drupalcode.org/project/menu_trail_by_path/-/blob/2.x/src/Men...

Should we update this to check if (empty($this->cid)) as per #6?

Reverted this from 10.3.x and 11.0.x for now. Didn't really expect there to be subclasses of specific cache collector implementations, would have been more reticent if I had.

Didn't really expect there to be subclasses of specific cache collector implementations

+1, quite surprised myself.

Okay so all 3 examples mentioned in #18 have the following code:

if (!isset($this->cid)) {

This will only work if $this->cid is NULL, the very thing this issue was trying to fix. I still agree that the original MR in this issue is correct, as we are passing the wrong data to a constructor right now. But it's now apparent that we cannot simply change this.

So I see two paths forward:

1. We open up the CacheCollector constructor to allow NULL as a cache ID (I'm not a fan of this)
2. We add a deprecation warning in CacheCollector's constructor that passing NULL is not allowed, typehint it as string according to the deprecation guidelines → and then re-commit this fix in the next major. In the meantime, downstream has plenty of time to change their NULL to '' and fix their check in getCid()

Other suggestions obviously also welcome.

I agree with the second option. The thing is that this is already in a release, so contrib probably needs to implement workarounds anyway. So maybe we can just leave it in 10.4 and 11.x and add the deprecations to 10.3 and 11.0

Rolled back from 10.4.x as well..

So changing what gets set in a constructor is 100% allowable in a minor release, it's just unfortunate that I backported this and then we had the out-of-schedule patch release for Twig a couple of days later, so not much time for it to get caught.

What I am thinking is:

1. Release a 'paper bag' release for 10.3 so that sites can just update again (or skip the previous one if they haven't already) and the contrib modules will be unbroken. If none of the three modules have 11.x compatible versions yet, we might not need an 11.0.x paper bag release, but can do that if we need to too.

2. Leave this change in 11.x (i.e. 11.1.0) but add a change record and open issues against any of the three modules that don't have one already.

But we could also add a deprecation in 11.1.x in #2 and change this again in 12.x to be even more cautious.

Menu Trail by Path does have an 11.x release so we should put out new patch releases of 10.3 and 11.0.

Given this is only cleanup to satisfy PHPStan and there is no functional requirement to do this sooner I vote for changing this to a deprecation in 11.1 and fixing properly in 12.

Given this is only cleanup to satisfy PHPStan and there is no functional requirement to do this sooner I vote for changing this to a deprecation in 11.1 and fixing properly in 12.

+1 making breaking changes for PHPStan's sake in minors is something we should avoid.

Just pushed the paper bag releases for 10.3 and 11.0.

Went ahead and reverted the commit from 11.x too given #26 and #28 so we've got a fresh slate here.

Okay, in that case:

1. The CacheCollector constructor needs to check the cache ID using is_string() and fire a deprecation when it's not
2. We can already add the commented out string typehint to the constructor
3. We need a follow-up for the next major to actually enforce the typehint
4. We need a CR for said new issue explaining the constructor change

Did I miss anything? If not, we can update the IS with those steps.

Thank you such much for this fixed issue!

It looks like the issue has been resolved with the recent merge and release. Can we mark the issue as fixed, or is there anything else pending that needs to be addressed?

@rajeshreeputra we need to do as per #31

Re #31. I think we could take a slightly different approach.

I think we could still allow $cid in the constructor to be NULL but if it is a NULL in this case not set $this->cid. And then in the docs say that if you construct this with NULL it's the responsibility of an override of \Drupal\Core\Cache\CacheCollector::getCid() to return a string. This way we can enforce stringy-ness of $this->cid but continue to use the lazy setting in the classes that override getCid().

Re #35

The current constructor has $this->cid = $cid;, so if we allow NULL then that code can remain as is, no? It's equivalent to not setting the property. So that would effectively mean we keep the current code as is, while only changing the @param docs for $cid to "string|null".

I'm fine with the concept of allowing a NULL $cid so that getCid() can take care of the actual computing of the string, though. But at that point we need to ask ourselves what the point is of accepting a $cid in the constructor. Couldn't we then better turn getCid() into an abstract method that has to be implemented? That way we're sure everyone returns a correct CID by virtue of typehinting the return value and phpstan verifying that.