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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

vakulrai’s picture

Status: Active » Needs review
FileSize
603 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
FileSize
1.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.