Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vegantriathlete created an issue. See original summary.

vegantriathlete’s picture

Assigned: vegantriathlete » Unassigned
Status: Active » Needs review
FileSize
22.6 KB

Patch attached!

vegantriathlete’s picture

Issue tags: +Quick fix, +Novice
vegantriathlete’s picture

I am wondering if I should have put four "Use . . . instead" statements in the docBlock for the clearCachedDefinitions() method. I have put just the one that is not supposed to be removed as noted in the @todo.

@see: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Entity/Entit...

Berdir’s picture

Pasted from IRC:


not sure, referencing them all would be the safe option, something like to clear cached entity types, use X, to clear cached entity bundle information, use X. to clear entity field definitions, use X. actually, the last can be left out, if anything then that's something that needs to happen automatically when caches are cleared, not a public api
also, the onX methods are event listeners, not an API that should be called directly, so IMHO, they don't really to have replacement information

Forget that, I think this is fine as it is.

The additional calls are all depending data on the entity type definitions and they should somehow listen to that cache clear and re-act to it and the previously public method have their own deprecated documentation.

joshmiller’s picture

Status: Needs review » Reviewed & tested by the community

I just checked and the patch applies and appears to have the right format with a good reason to think it's good from Berdir.

Sounds like an RTBC.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

We are in the process of adding @see links to these as well, so it would be appropriate to do it here I think instead of needing to change all of them again. The @see would be a URL to the change record where this change was made.

naveenvalecha’s picture

vegantriathlete’s picture

naveenvalecha’s picture

Status: Needs review » Needs work

Thanks, @vegantriathlete for rerolling the patch. We're near to RTBC. only few nitpicks needs to be addressed

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -25,6 +25,9 @@ class EntityManager implements EntityManagerInterface, ContainerAwareInterface {
+   * @see https://www.drupal.org/node/2549139

Add the @see statement after the Use and add a new line space b/w @see and Use. We need to change it across all places.

//Naveen

Dinesh18’s picture

Status: Needs work » Needs review
FileSize
27.64 KB
25.3 KB

Here is an updated patch which implements #10

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @Dinesh18! This is the first Quickfix of the month which I'm RTBCing

//Naveen

Gábor Hojtsy’s picture

Title: Correct @deprecated docBlock in EntityManager.php » Expand @deprecated docblocks in EntityManager.php

  • Gábor Hojtsy committed 5902695 on 8.3.x
    Issue #2867882 by vegantriathlete, Dinesh18, naveenvalecha, Gábor Hojtsy...

  • Gábor Hojtsy committed fb746e5 on 8.4.x
    Issue #2867882 by vegantriathlete, Dinesh18, naveenvalecha, Gábor Hojtsy...
Gábor Hojtsy’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for the update, now this looks much more complete :)

Status: Fixed » Closed (fixed)

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

Mile23’s picture