Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stalski’s picture

Project: D8 Field API » Drupal core
Version: » 8.x-dev
Component: Code » field system
Assigned: Unassigned » Stalski

I start working on this, as of NOW

Stalski’s picture

Status: Postponed » Active

forgot status change

Stalski’s picture

Status: Active » Needs review
FileSize
8.22 KB
Stalski’s picture

FileSize
8.19 KB

Removed trailing spaces, sorry

Status: Needs review » Needs work

The last submitted patch, 1787248_4.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review

Triggering testbot again

pcambra’s picture

#4: 1787248_4.patch queued for re-testing.

Stalski’s picture

FileSize
8.16 KB

Found a comment that was not correct.

webflo’s picture

Looks good. I found one minor issue.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/RSSCategoryFormatter.phpundefined
@@ -0,0 +1,48 @@
+          'domain' => $item['tid'] != 'autocreate' ? url('taxonomy/term/' . $item['tid'], array('absolute' => TRUE)) : '',

Lets use entity uri $item['taxonomy_term']->uri() for consistency.

webflo’s picture

Removed the taxonomy_field_formatter_* hooks and switched to $item['taxonomy_term']->uri() in RSSCategoryFormatter.

Stalski’s picture

great webflo, thx a lot!

Sweetchuck’s picture

Nothing new, just it works.
The patch #10 is not applicable to the latest 8.x.

Status: Needs review » Needs work

The last submitted patch, taxonomy-formatters-plugins-1787248.patch, failed testing.

yched’s picture

Priority: Normal » Major

Bumping, this is now a blocker for #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.

Anyone up for reviving this ?

swentel’s picture

Issue tags: +Field API

Crap, completely forgot about this one

jibran’s picture

Status: Needs work » Needs review
FileSize
12.84 KB

Here is the re-roll lets see how much it fails.

swentel’s picture

I even think it might be green - or at least really close. 2 remarks regarding documentation we need to fix

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/LinkFormatter.phpundefined
@@ -0,0 +1,60 @@
+ * Definition of Drupal\taxonomy\Plugin\field\formatter\LinkFormatter.

Should be Contains \Drupal (apparently with leading forward slash now right ?)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/LinkFormatter.phpundefined
@@ -0,0 +1,60 @@
+   * Implements Drupal\field\Plugin\Type\Formatter\FormatterInterface::viewElements().

Should all be {@inheritdoc}

swentel’s picture

oh and thank you also for the re-roll! :)

Status: Needs review » Needs work

The last submitted patch, taxonomy-formatters-plugins-1787248-16.patch, failed testing.

ParisLiakos’s picture

i think thats why this was forgotten
#1847596: Remove Taxonomy term reference field in favor of Entity reference which is postponed on #1818560: Convert taxonomy entities to the new Entity Field API
maybe we should postpone or duplicate it

swentel’s picture

As far as I can see, #1818560: Convert taxonomy entities to the new Entity Field API wouldn't conflict that much (maybe not at all), so I'd rather go ahead with this one because I think the other one is going to be a while for that goes in I'm afraid.

yched’s picture

Agreed with @swentel. Most of the job is done already, and getting to a point where there are no legacy widgets and formatters will be precious to unblock other work.

ParisLiakos’s picture

Issue tags: +Plugin system

the other one is going to be a while for that goes in I'm afraid

agreed..
tagging

nils.destoop’s picture

Changed the documentation, and tests should be fixed.

nils.destoop’s picture

Status: Needs work » Needs review
effulgentsia’s picture

Status: Needs review » Patch (to be ported)

#24 looks great. Just some nits:

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/LinkFormatter.php
@@ -0,0 +1,60 @@
+    // Terms whose tid is 'autocreate' do not exist
+    // yet and $item['taxonomy_term'] is not set. Theme such terms as
+    // just their name.

The code below this uses $item['entity'] instead of $item['taxonomy_term'], so let's update the comment accordingly. Also, I think the line breaks can be adjusted so that this fits on two lines.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/PlainFormatter.php
@@ -0,0 +1,49 @@
+    // Terms whose tid is 'autocreate' do not exist
+    // yet and $item['taxonomy_term'] is not set. Theme such terms as
+    // just their name.

Ditto

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/RSSCategoryFormatter.php
@@ -0,0 +1,59 @@
+    // Terms whose tid is 'autocreate' do not exist
+    // yet and $item['taxonomy_term'] is not set. Theme such terms as
+    // just their name.

And once more.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/TaxonomyFormatterBase.php
@@ -0,0 +1,75 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function viewElements(EntityInterface $entity, $langcode, array $items) {}

Is there a reason for an empty implementation of this on the base class? If not, let's remove it and make the class abstract.

effulgentsia’s picture

Status: Patch (to be ported) » Needs work
pcambra’s picture

Assigned: Stalski » Unassigned
Status: Needs work » Needs review
FileSize
12.4 KB

Here's a reroll with @effulgentsia comments applied, I don't see a reason to the empty implementation either so I've removed it.

swentel’s picture

Two nitpicks, after that it's good.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/TaxonomyFormatterBase.phpundefined
@@ -0,0 +1,70 @@
+      // Iterate through the fieldable entities again to attach the loaded term data.

80 chars limit

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/formatter/TaxonomyFormatterBase.phpundefined
@@ -0,0 +1,70 @@
+          // Check whether the taxonomy term field instance value could be loaded.

same here

pcambra’s picture

Here you go :)

Status: Needs review » Needs work
Issue tags: -Field API, -Plugin system

The last submitted patch, taxonomy-formatters-plugins-1787248-30.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: +Field API, +Plugin system
effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

#26 and #29 addressed, so RTBC. Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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