Problem/Motivation

In working on Ubercart 7 to Commerce 2 I came across a reason to extend Field.php but it is marked @internal. The reason stems from the fact that products are nodes in Ubercart 7 but commerce_products in Commerce 2. If a field exists on a product node and another non product node type the Commerce 2 sites needs field storage on the node entity and the commerce_product entity. To do this adding a new row to the field plugin mimicking that the field existed in the source on a commerce_product entity works well.

Field.php was marked internal in #2891932: Refactor D7 Field source plugin and the references to the decision are in comments in #10 and #11. It states that @internal was used to keep some flexibility if this needed to be changed in the future.

The plugin was refactored in 8.4.x, there are have been 3 changes to the file since then, 2 for fixes to the annotation and 1 to add handling of Drupal 7 title data.

Proposed resolution

Since nothing has turned up yet to indicate that this needs to remain internal, let's remove it.

Remaining tasks

Patch that removes the following for d7/Field.php

 * @internal
 *
 * This class is marked as internal and should not be extended. Use
 * Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase instead.
 *

Review
Commit

API changes

The source plugin is no longer @internal.

Marking novice for making the initial patch as described in 'Remaining Tasks'.

CommentFileSizeAuthor
#5 2996012-5.patch1.23 KBanmolgoyal74
#2 2996012-2.patch603 bytesvakulrai

Comments

quietone created an issue. See original summary.

vakulrai’s picture

Status: Active » Needs review
StatusFileSize
new603 bytes

Adding a patch for the same.

maxocub’s picture

Should we do the same for Drupal\field\Plugin\migrate\source\d7\FieldInstance for consistency's sake?

heddn’s picture

Status: Needs review » Needs work

Yeah, let's do it for field instance too. Back to NW and still leaving the novice tag as that should be pretty strait forward.

anmolgoyal74’s picture

Status: Needs work » Needs review
StatusFileSize
new1.23 KB
maxocub’s picture

Status: Needs review » Reviewed & tested by the community

This looks good, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2996012-5.patch, failed testing. View results

th_tushar’s picture

Status: Needs work » Reviewed & tested by the community

Marking it as RTBC, as patch cleared testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed c3e26c4 and pushed to 8.7.x. Thanks!

  • catch committed c3e26c4 on 8.7.x
    Issue #2996012 by anmolgoyal74, vakulrai, quietone, maxocub: Remove @...

Status: Fixed » Closed (fixed)

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