Follow up from #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system. That issue moved formatters and widgets, but not field types, because according to #2050835-34: Move Widget, Formatter, and FieldType plugin types to the Core\Field system, moving field types might be better to do after #2047229: Make use of classes for entity field and data definitions is done.

However, we now have a situation where both Plugin/field and Plugin/Field directories exist, which may be a problem on case-insensitive or case-semisensitive file systems. Marking this issue critical for that, unless someone knows for sure that it's not a problem.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

That might be a pretty serious problem actually, has anyone tested to clone/pull on Windows since this got in? I think it's not part of alpha4, right?

smiletrl’s picture

I get this problem, after git pull
http://snag.gy/qzRro.jpg

Berdir’s picture

Yeah, thought so, that's not good, we might want to revert.

smiletrl’s picture

Or start to move field types?

I personally don't see much conflict with #2047229: Make use of classes for entity field and data definitions. But it would be definitely good to move the last two field types #2015687: Convert field type to FieldType plugin for taxonomy module and #2015689: Convert field type to typed data plugin for options module asap.

Berdir’s picture

There might be fewer conflicts now, as it doesn't contain the list_type changes anymore, yes, so we could possibly just move them.

Berdir’s picture

Status: Active » Needs review
FileSize
178.13 KB
44.05 KB

Ok...

Here's an attempt to remove those, let's see what the testbot has to say.

Two patches. The first just contains the necessary renames to get rid of the Field/field propblem. The second one also moves \Drupal\Core\Entity\Field and \Drupal\field\Plugin\Type\FieldType into \Drupal\Core\Field. Especially the second part is huge and changes all formatters and widgets *again*, as they all type hint on the classes that we are moving. And will conflict with quite a few patches.

The second patch also fixes all implements/overrides that use the old class names to @inheritdoc and removes a few now unnecessary use's.

Providing both as I have two commits anyway, we could go with the first one, but in a way, it would be backwards because the second would then touch most changes again and break things a second time.

Status: Needs review » Needs work

The last submitted patch, move-all-the-field-type-classes-2115145-6.patch, failed testing.

Berdir’s picture

Missed on class and the parent:: thing seems like a PHPStorm move bug. Missed a few namespace changes in the big patch. This should do better.

interdiff for each patch, having fun with juggling commits around with git rebase...

Berdir’s picture

Status: Needs work » Needs review
amateescu’s picture

Review of the big one. It's just PHPStorm doing its thing..

  1. +++ b/core/lib/Drupal/Core/Field/ConfigEntityReferenceItemBase.php
    @@ -21,7 +20,7 @@
    +class ConfigEntityReferenceItemBase extends EntityReferenceItem implements \Drupal\Core\Field\ConfigFieldItemInterface {
    

    .

  2. +++ b/core/modules/file/lib/Drupal/file/Plugin/Field/FieldType/FileFieldItemList.php
    @@ -23,7 +23,7 @@ public function defaultValuesForm(array &$form, array &$form_state) { }
    -    parent::insert();
    +    \Drupal\Core\Field\parent::insert();
    
    @@ -92,7 +92,7 @@ public function delete() {
    -    parent::deleteRevision();
    +    \Drupal\Core\Field\parent::deleteRevision();
    

    :)

  3. +++ b/core/modules/file/lib/Drupal/file/Plugin/Field/FieldType/FileItem.php
    @@ -31,10 +31,10 @@
    +class FileItem extends \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem implements \Drupal\Core\Field\ConfigFieldItemInterface {
    

    .

  4. +++ b/core/modules/number/lib/Drupal/number/Plugin/Field/FieldType/NumberItemBase.php
    @@ -2,17 +2,17 @@
    +abstract class NumberItemBase extends \Drupal\Core\Field\ConfigFieldItemBase {
    

    .

  5. +++ b/core/modules/telephone/lib/Drupal/telephone/Plugin/Field/FieldType/TelephoneItem.php
    @@ -21,7 +21,7 @@
    +class TelephoneItem extends \Drupal\Core\Field\ConfigFieldItemBase {
    

    .

  6. +++ b/core/modules/text/lib/Drupal/text/Plugin/Field/FieldType/TextItemBase.php
    @@ -2,18 +2,18 @@
    +abstract class TextItemBase extends \Drupal\Core\Field\ConfigFieldItemBase implements PrepareCacheInterface {
    

    .

Berdir’s picture

Yep, PHPStorm is storming.. no idea why it messes some up...

Status: Needs review » Needs work

The last submitted patch, move-all-the-field-type-classes-2115145-11.patch, failed testing.

Berdir’s picture

A few fixes.

Status: Needs review » Needs work

The last submitted patch, move-all-the-field-type-classes-2115145-13.patch, failed testing.

Berdir’s picture

Fixed the last parent:: and two inline-namespace lines.

Berdir’s picture

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now.

alexpott’s picture

Title: Move field type plugins to Plugin/Field/FieldType » Change notice: Move field type plugins to Plugin/Field/FieldType
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +API change, +Needs change record, +Field API, +Entity Field API, +Approved API change

Committed 3ded221 and pushed to 8.x. Thanks!

yched’s picture

W00t! Awesome, thanks guys :-)

The plugin managers look like they're still exposed by field.module's services.yml ? (same for the cache bins they use)

Berdir’s picture

#2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system already moved them to core.services.yml, field type was actually already there?

yched’s picture

Oh, ok - sorry, still away from my coding env till tomorrow :/

smiletrl’s picture

Status: Active » Needs review
Issue tags: -Needs change record

Here's the change record https://drupal.org/node/2119163. It only tells the folders of discovery change.

yched’s picture

I don't think this should deserve a change notice, this is a D8/D8 change. Rather, existing change notices about widgets / formatters / field types as plugins should be updated ?

Berdir’s picture

Yes, I'm working on updating those right now.

I thought a separate change notice was unnecessary at first too, but we started writing short change notices about HEAD to HEAD changes, just to get a notification out. Quite a few people are already working on porting/writing 8.x modules. I'll probably shorten it down a bit, things like where services are defined aren't relevant.

Berdir’s picture

Title: Change notice: Move field type plugins to Plugin/Field/FieldType » Move field type plugins to Plugin/Field/FieldType
Priority: Major » Critical
Status: Needs review » Fixed
yched’s picture

Thanks a lot :-)

HEAD to HEAD change notifs: true, notifications are cool - I'm just a bit worried that people browsing the collection of change notifs have no way of telling the difference (7/8 changes vs 8/8 changes) ?

Status: Fixed » Closed (fixed)

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