Fix migrate destination entity PHPStan L0 issues

Are EntityConfigBase and EntityContentBase the only two valid immediate child classes, and all users are expected to only extend them? If so we could make these abstract now?

Should this go back to NW for discovery?

Free to review once it's done. If I don't get to it today just ping me on the #needs-review-queue channel and can take a look

Changes in #10 look good to me.

I looked at where these two methods are used in core to confirm that these are correct additions.

1. EntityContentBase, which extends this class, has its own docblock for processStubRow(). It can now be replaced by {@inheritdoc}. (And yes, Symfony is dropping that, but we haven't adopted it ourselves so out of scope here to change the coding standard.) 🙂
    

2. Similarly, EntitySearchPage::updateEntity() has its own docblock that can be replaced by {@inheritdoc}.
    

3. EntityRevisionTest::updateEntity() also has its own docblock, but in that case it's overriding the default behavior (and not calling the parent) so it's justified. However, that implementation's docblock does need the parameter and return docs added.

4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -180,6 +181,29 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +   * Updates the entity with the contents of a row.
   +   *
   +   * This method should be implemented in extending classes.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity to update.
   +   * @param \Drupal\migrate\Row $row
   +   *   The row object to update from.
   +   */
   +  abstract protected function updateEntity(EntityInterface $entity, Row $row);
   

   It appears that the implementations return $entity, and are expected to do so by callers. So, we should add that return value to the docs.

   The EntityContentBase implementation also has a @throws that is missing here and may be relevant.

5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -180,6 +181,29 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +  protected function processStubRow(Row $row) {
   +  }
   

   The closing curly for this should be on the same line.

Thanks for working on this! It's great how many bugs PHPStan is finding in our APIs.

Saving credits for the patch and substantive reviews.

*cough*
PHPStan errors
*cough*

Appears that changes requested in #12 have been addressed.

Thanks @mondrake. I think it's good to include #12.4 here, at least in 10.1.x. Maybe we should add a small change record for it, though.

It looks like the point about the @throws from #12.4 also hasn't been addressed yet, though. Thanks!

Come to think of it, this whole issue should get a CR.

Sorry, it was EntityConfigBase:

   * @throws \LogicException                                                    
   *   Thrown if the destination is for translations and either the "property"  
   *   or "translation" property does not exist.   

Re:

I do not understand what is changing here that requires a CR. Per #5, the classes that should be extended are EntityConfigBase and EntityContentBase, and they're not changing. If now someone is extending Entity directly without implementing updateEntity(), they would fail anyway.

We are adding an API and changing a best practice. That gets a change record. It doesn't have to be a public BC break to get a change record.

For the change record

Rerolled 3318888-16.patch on 11.x in MR

Added draft CR, definitly needs some eyes on that one.

CR looks good to me, especially since there is nothing for contrib to update.

+   * This method should be implemented in extending classes.

This is the definition of an abstract method, so there is probably no need to explicitly say it.

+   * @throws \LogicException
+   *   Thrown for config entities, if the destination is for translations and
+   *   either the "property" or "translation" property does not exist.

This is describing implementation details, which don't relate to this specific base class.

However, I don't think either of these should hold up commit, so let's just get this in. Committed and pushed 62d7c46774 to 11.x (10.2.x), and published the change record. Thanks!

This is describing implementation details, which don't relate to this specific base class.

Fully agreed, but couldn't see how to address #12.4 and #22.1

Closed #2923683: \Drupal\migrate\Plugin\migrate\destination\Entity calls methods that are not required by subclasses as a duplicate, adding credit.