inaccurate description for EntityTypeManager::getHandler()

Adding a patch.

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

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

More clear for me.

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!

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".

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.

Attached is a new wording mentioning the caching.

+++ 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.

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!

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

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

Brackets are removed.

@joachim thanks.

Adding description from #25

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

Added the first line, with the interdiff against 27!

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

@munavijayalakshmi Screenshots of code are really not helpful.

@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.

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

RTBCing was fine though.
Reverting back to RTBC

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!

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.

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.

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

.

Updating issue credit to add @joachim and @xjm who were instrumental in shaping the patch and provided in-depth mentoring - thanks!

Hiding some outdated files.

+++ 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.

I agree with @larowlan.

Here is an updated patch and interdiff

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