Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vivianspencer created an issue. See original summary.

tstoeckler’s picture

Issue tags: +Novice

Good find, thanks!

sidharthap’s picture

Status: Active » Needs review
FileSize
759 bytes

Initial Try.

tstoeckler’s picture

Status: Needs review » Needs work

@sidharthap: Thanks for the patch. Since this is only ever used for documentation and not actually code that is executed we can directly change it without any deprecation notice though.

sidharthap’s picture

@tstoeckler thank you. is it mean we can directly remove cacheUntilEntityChanges() from the code by using addCacheableDependency()
for Version: 8.0.x-dev also?

tstoeckler’s picture

@sidharthap: Yup, exactly.

sidharthap’s picture

Thank you @tstoeckler
Updated patch as per #6. it is only remove from block.api.php.

sidharthap’s picture

Status: Needs work » Needs review
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yay, thanks a lot. Looks great.

alexpott’s picture

Title: cacheUntilEntityChanges in the example is deprecated » cacheUntilEntityChanges() is deprecated and should be replaced with addCacheableDependency()
Component: documentation » cache system
Status: Reviewed & tested by the community » Needs work

See https://www.drupal.org/core/scope - we should replace all the usages of cacheUntilEntityChanges() in core so we are consistent.

thhafner’s picture

@alexpott

Agreed. Should a new issue be opened or should the novice tag be removed from this issue?

alexpott’s picture

I changed the issue title - so let's use this one. Afaics this is still a novice issue.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
30.99 KB

updated patch, removed from all the files as per comment #10. I think we can have a separate issue to remove the function cacheUntilEntityChanges() from
core\lib\Drupal\Core\Access\AccessResult.php file.

tstoeckler’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Needs review » Needs work
$ grep 'cacheUntilEntityChanges' -R .
./core/lib/Drupal/Core/Access/AccessResult.php:  public function cacheUntilEntityChanges(EntityInterface $entity) {

The patch itself is great. However, it only applies to 8.0. We should get this in to 8.2 first, so the patch needs to be rerolled.
I personally think we should at least fix the docs in 8.1 and 8.0 as well, but not sure what others think.

thhafner’s picture

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yup, this one is good to go for 8.2. (It applies to 8.1 as well, but I guess it's too late for that, if I'm not mistaken.)

$ grep 'cacheUntilEntityChanges' -R . | wc -l
52
$ curl https://www.drupal.org/files/issues/updating-deprications-2686765-15.patch | git apply --index
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 31740  100 31740    0     0  47318      0 --:--:-- --:--:-- --:--:-- 47302
$ grep 'cacheUntilEntityChanges' -R . | wc -l
1
$ grep 'cacheUntilEntityChanges' -R .
./core/lib/Drupal/Core/Access/AccessResult.php:  public function cacheUntilEntityChanges(EntityInterface $entity) {

  • catch committed fb8d5b1 on 8.2.x
    Issue #2686765 by sidharthap, thhafner: cacheUntilEntityChanges() is...
catch’s picture

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

Committed/pushed to 8.2.x, thanks!

I'd personally put this into 8.1.x as well since we have a second beta to put out still. It's not newly deprecating anything, just removing deprecated usages. But leaving RTBC against 8.1.x in case someone else thinks it is too late.

One advantage of having this in 8.1.x is it will allow us to actively deprecate the function in 8.2.x with trigger_error() if we want.

tstoeckler’s picture

Sure, even better!

Wim Leers’s picture

Issue tags: +D8 cacheability

Yay! Thanks :)

+1 for 8.1.x.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: updating-deprications-2686765-15.patch, failed testing.

sashi.kiran’s picture

Assigned: Unassigned » sashi.kiran
Issue tags: +Needs reroll

I am re-rolling this patch.

alexpott’s picture

Assigned: sashi.kiran » Unassigned
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

@sashi.kiran it applies to 8.1.x so there is no need to do a reroll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: updating-deprications-2686765-15.patch, failed testing.

The last submitted patch, 15: updating-deprications-2686765-15.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

More unrelated test fails..

  • catch committed cb6362a on 8.1.x
    Issue #2686765 by sidharthap, thhafner: cacheUntilEntityChanges() is...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Taking lack of comment as agreement to put this in 8.1.x too, so cherry-picked there as well. Thanks all.

tstoeckler’s picture

Status: Fixed » Needs review

So the original bug report, i.e. the false documentation on https://api.drupal.org/api/drupal/core!modules!block!block.api.php/funct... is not yet fixed. I don't know in how far api.drupal.org already supports the semantic versioning and what the plans are regarding that, but IMHO it would be nice to commit #7 (i.e. just the docs change) to 8.0, as well. Thoughts?

alexpott’s picture

I don't think fixing this documentation in 8.0.x is necessary... as far as I remember api.d.o is already on 8.1.x

catch’s picture

Status: Needs review » Fixed

Back to fixed.

tstoeckler’s picture

Status: Fixed » Needs review

Sorry to be somewhat obnoxious, but visiting https://api.drupal.org/api/drupal/core!modules!block!block.api.php/funct... still gives the old code, so not sure about #30

alexpott’s picture

@tstoeckler you have to wait for it run cron I don't think it reflects changes immediately

thhafner’s picture

Why did the patch not apply properly?

tstoeckler’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

You're right about api.drupal.org, however there's an issue to fix it at #2662812: Figure out how to deal with multiple 8.x branches.

8.0.x won't be getting any further commits unless there's a security release, so moving this to closed again.

tstoeckler’s picture

Ahh OK, fine. Thanks for the link.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.