API page: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!EntityType...

> Creates a new handler instance for a entity type and handler type.

That's not true. It caches the instantiated handlers and only creates one if needed.

Rather than say 'creates', the documentation should explicitly state that handlers are per entity type, not per entity.

This is so developers know that they may statically cache something in handler, but also to ensure the cache is per entity if necessary.

Comments

joachim created an issue. See original summary.

gianani’s picture

Assigned: Unassigned » gianani
gianani’s picture

Status: Active » Needs review
FileSize
689 bytes

Adding a patch.

arunkumark’s picture

The patch #3 looks good. Some more detailed description added to it and created a new patch.

ZeiP’s picture

Assigned: gianani » Unassigned
FileSize
696 bytes

Attached is a patch for a slightly reworded version fixing a small spelling mistake.

jungle’s picture

Status: Needs review » Reviewed & tested by the community

More clear for me.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this patch to improve Drupal's documentation.

This docblock does not follow the documentation standards. The summary should be a single line of 80 characters or fewer. The restriction is deliberate, in order to make sure that function summaries are succinct and clear.

Reference: https://www.drupal.org/node/1354

So, we should try to reword this to fit in a single 80-character line. If necessary, we can add subsequent (longer) paragraphs, but I don't think that's necessary here.

Thanks!

jungle’s picture

It's a method in an interface, IMO, short description is enough.

- Creates a new handler instance for a entity type and handler type.
+ Gets a handler instance for the entity and handler type.

Leave the details to the implementation, so omit here "creating a new one if one doesn't exist".

jungle’s picture

Status: Needs work » Needs review
jungle’s picture

Change a little bit.

- Gets a handler instance for the entity and handler type.
+ Gets a handler instance for a entity type and handler type.

joachim’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityTypeManagerInterface.php
@@ -93,7 +93,7 @@ public function getRouteProviders($entity_type);
-   * Creates a new handler instance for a entity type and handler type.
+   * Gets a handler instance for a entity type and handler type.

That's not addressing this issue at all.

> Leave the details to the implementation, so omit here "creating a new one if one doesn't exist".

That's not an implementation detail. Handlers rely on this behaviour when they cache data in themselves, so it has to be part of the interface's specification.

ZeiP’s picture

Status: Needs work » Needs review
FileSize
662 bytes

Attached is a new wording mentioning the caching.

jungle’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Entity/EntityTypeManagerInterface.php
@@ -93,7 +93,7 @@ public function getRouteProviders($entity_type);
-   * Creates a new handler instance for a entity type and handler type.
+   * Gets a new or existing handler instance for the entity and handler type.

Accurate and followed the documentation standards.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @joachim, @ZeiP, and @jungle!

Based on #11 and a re-read of the summary, I do think that we should add a second paragraph that retirates that it will either use an existing one or create a new one otherwise (if that's correct), and that also addresses:

the documentation should explicitly state that handlers are per entity type, not per entity.

So something like:

/**
 * Gets a handler instance for the handler and entity type.
 *
 * Handlers are created and cached per entity type, so [what
 * might happen if you expect it to per entity]. If a handler 
 * instance is not already instantiated for the entity type, 
 * a new one will be created and returned. 

That's just an example and I haven't reviewed the code in depth -- so please use that example just as a suggested format and improve on the actual documentation.

Thanks!

arunkumark’s picture

As per comment #14 i have re-pached. @xjm thanks for comment.

joachim’s picture

Status: Needs review » Needs work

The bit in square brackets is a placeholder! ;)

arunkumark’s picture

Status: Needs work » Needs review
FileSize
895 bytes

As per comment 16 the patch was re-rolled,
@joachim thanks for your comment.

Status: Needs review » Needs work

The last submitted patch, 17: get_handler-2820416-17.patch, failed testing.

joachim’s picture

That still has the text in square brackets!

arunkumark’s picture

Brackets are removed.

@joachim thanks.

arunkumark’s picture

Status: Needs work » Needs review
joachim’s picture

Status: Needs review » Needs work

Right but the text that was in those needs WRITING!!! It was just a placeholder!

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.

shashikant_chauhan’s picture

joachim’s picture

How about this for description text:

Entity handlers are instantiated once per entity type and then cached in the entity type manager, and so subsequent calls to getHandler() for a particular entity type and handler type will return the same object. This means that properties on a handler may be used as a static cache, although care must be taken with any data that is specific to a particular entity.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
1.86 KB

Adding description from #25

tameeshb’s picture

FileSize
987 bytes
733 bytes

Some irrelevant changes came along in #26,
Patch redone with interdiff against #26

joachim’s picture

Status: Needs review » Needs work

Thanks. Quick work!

What you're adding looks good, but you shouldn't be removing the first line of the docs. All docblocks should start with a paragraph of a single line. The one that is already here is fine and should stay.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
973 bytes
657 bytes

Added the first line, with the interdiff against 27!

joachim’s picture

Patch looks perfect. Thanks!

Though someone else should review it, since it's me that wrote the text.

Munavijayalakshmi’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
23.24 KB
34.81 KB

Applied patch core/lib/Drupal/Core/Entity/EntityTypeManagerInterface.php cleanly.

himanshu-dixit’s picture

@munavijayalakshmi Screenshots of code are really not helpful.

tameeshb’s picture

@Munavijayalakshmi Screenshots of "before" and "after" are attached when there has been some UI changes in the issue. In that case we post screenshots of the UI components that have changed before and after.
Code is text, and there is no use of taking screenshots of code snippets.

Munavijayalakshmi’s picture

Status: Reviewed & tested by the community » Needs review

@tameeshb thank you for your suggestion. I am new to drupal.

tameeshb’s picture

Status: Needs review » Reviewed & tested by the community

RTBCing was fine though.
Reverting back to RTBC

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @joachim for providing mentoring on this issue. Going to add my own again now: for @tameeshb, the patch should only be marked RTBC by someone who has performed the review task. Creating patches for Drupal is not just a matter of following instructions; we need your contributions to include your thoughtful evaluation of what the best change is and whether it is correct. Also, as a best practice, you should not mark any patch that you create as RTBC.

In #30, joachim acknowledges that he wrote the text, so his proposed text needs to be reviewed for its accuracy and clarity by a peer reviewer. So, the way to provide a review for (and then RTBC) this patch is to read the documentation, read the code describes, and confirm that what the code does is properly explained by the documentation.

Thanks!

xjm’s picture

Status: Needs review » Needs work

Here is an example of what I might ask when reviewing the documentation:

+++ b/core/lib/Drupal/Core/Entity/EntityTypeManagerInterface.php
@@ -95,6 +95,13 @@ public function hasHandler($entity_type, $handler_type);
+   * although care must be taken with any data that is specific to a
+   * particular entity.

What kind of care must be taken? As a developer, this does not give me enough information to know what I need to do when using this method for my entity data.

joachim’s picture

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

> What kind of care must be taken? As a developer, this does not give me enough information to know what I need to do when using this method for my entity data.

I'm not really sure what we can say that's not going to be way too specific. What I am trying to say here basically is that if you store data in the handler by doing $this->mystuff = 'cats', you need to be sure that 'cats' applies to all entities of this type, and if not, do $this->mystuff[ENTITY ID] = 'cats'.

Perhaps we can say something like:

> This means that properties on a handler may be used as a static cache, although as the handler is common to all entities of the same type, any data that is per-entity should be keyed by the entity ID.

-- but I feel that's rather too bogged down in implementation details.

gaurav.kapoor’s picture

This means that properties on a handler may be used as a static cache, although as the handler is common to all entities of the same type, any data that is per-entity should be keyed by the entity ID.

This looks appropriate to me.

pk188’s picture

Status: Needs work » Needs review
FileSize
1020 bytes

Update the patch accordingly to #38 and #39.
Check once please.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

Perfect! Thanks for working on the patch.

I'm taking the liberty of setting to RTBC despite having had a hand in writing the text, as my initial text was reviewed already in the previous patch and my amendment was reviewed in #39.

pk188’s picture

.

larowlan’s picture

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Entity/EntityTypeManagerInterface.php
@@ -95,6 +95,13 @@ public function hasHandler($entity_type, $handler_type);
    * Creates a new handler instance for a entity type and handler type.

Unless I'm mistaken, the original intent of this issue was that the short description was inaccurate. But we're not changing it here? Is that correct?

Shouldn't this now read something like
Returns a handler instance for the given entity type and handler

From what I can gather, the original issue summary objected to the use of the word 'new' but we've retained it.

Dinesh18’s picture

I agree with @larowlan.

Here is an updated patch and interdiff

joachim’s picture

Status: Needs review » Reviewed & tested by the community

Good catch!
Thanks for the updated patch.

I'd say this is ready now.

  • larowlan committed 4f1d0f7 on 8.4.x
    Issue #2820416 by arunkumark, tameeshb, jungle, ZeiP, Dinesh18, gianani...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 4f1d0f7 and pushed to 8.4.x. Thanks everyone.