Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

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

Let's try this

aspilicious’s picture

Status: Active » Needs review
FileSize
4.96 KB

hmm lets see what this does

aspilicious’s picture

Status: Needs review » Needs work

Needs work (and a test)

aspilicious’s picture

Status: Needs work » Needs review
FileSize
5.11 KB

Writing tests can be a pain...

Status: Needs review » Needs work

The last submitted patch, options_formatter_tests.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
7.86 KB

Another try...

Status: Needs review » Needs work

The last submitted patch, options_formatter_full.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
7.99 KB

I almost forgot to finish this. Shame on me.

aspilicious’s picture

Assigned: aspilicious » Unassigned

Ok green! :)
I don't have a lot of time to reroll this if needed so I'm unassigning.
I think this is actually done (or very close to done).

nils.destoop’s picture

Looks ok for me.

2 small remarks:

you left a debug in testNodeDisplay:

debug('Saved ' . $this->field_name . ' configuration.');

The comment

// Old options

on line 42 in OptionsDefaultFormatter can be removed

After that, RTBC :)

aspilicious’s picture

Should be ok now :)

aspilicious’s picture

Didn't apply anymore, can we get this in?

aspilicious’s picture

Wrong upload...

dawehner’s picture

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/formatter/OptionsDefaultFormatter.phpundefined
@@ -0,0 +1,54 @@
+ * Definition of Drupal\options\Plugin\field\formatter\OptionsDefaultFormatter.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/formatter/OptionsKeyFormatterundefined
@@ -0,0 +1,45 @@
+ * Definition of Drupal\options\Plugin\field\formatter\OptionsKeyFormatter.

Contains ...

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/formatter/OptionsDefaultFormatter.phpundefined
@@ -0,0 +1,54 @@
+ *   id = "list_default",

Should we maybe start to prefix the plugin IDs with the module for formatters as well?

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/formatter/OptionsDefaultFormatter.phpundefined
@@ -0,0 +1,54 @@
+ *     "list_boolean",

Better not put an ending "," here.

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

Missing starting "\"

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/formatter/OptionsKeyFormatterundefined
@@ -0,0 +1,45 @@
+   * Implements Drupal\field\Plugin\Type\Formatter\FormatterInterface::viewElements().

"\"

+++ b/core/modules/options/lib/Drupal/options/Tests/OptionsFieldUITest.phpundefined
@@ -33,11 +33,12 @@ function setUp() {
+    $admin_user = $this->drupalCreateUser(array('access content', 'administer taxonomy', 'access administration pages', 'administer site configuration', 'administer users', 'administer permissions', 'administer content types', 'administer nodes', 'bypass node access'));

Does the user really need all this permissions? For example administer users doesn't seem to be required?

+++ b/core/modules/options/lib/Drupal/options/Tests/OptionsFieldUITest.phpundefined
@@ -33,11 +33,12 @@ function setUp() {
+    $this->type_name = $type_name;

Shouldn't be document this new variable?

+++ b/core/modules/options/lib/Drupal/options/Tests/OptionsFieldUITest.phpundefined
@@ -286,4 +287,53 @@ function assertAllowedValuesInput($input_string, $result, $message) {
+    $this->assertText('Saved ' . $this->field_name . ' configuration.', t("The 'On' and 'Off' form fields work for boolean fields."));

No t() function for assertion messages needed.

+++ b/core/modules/options/lib/Drupal/options/Tests/OptionsFieldUITest.phpundefined
@@ -286,4 +287,53 @@ function assertAllowedValuesInput($input_string, $result, $message) {
+
+    // Check the node page and see if the values are correct

Missing dot at the end.

+++ b/core/modules/options/lib/Drupal/options/Tests/OptionsFieldUITest.phpundefined
@@ -286,4 +287,53 @@ function assertAllowedValuesInput($input_string, $result, $message) {
+  }
 }

There should be a new line at the end of the file.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/formatter/OptionsDefaultFormatter.phpundefined
@@ -0,0 +1,54 @@
+ * Definition of Drupal\options\Plugin\field\formatter\OptionsDefaultFormatter.

Nitpick: Contains \ ...

aspilicious’s picture

Another try :)

dawehner’s picture

You seemed to have miss a lot of the points I made :)

aspilicious’s picture

I'm so tired... Uploaded wrong patch

Status: Needs review » Needs work

The last submitted patch, 1787238-options-formatters-17.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
6.91 KB
8.28 KB

We need this for #1875992: Add EntityFormDisplay objects for entity forms - so new patch here, hopefully green and a fast RTBC :)
Somehow you missed the extension also in the patch :)

aspilicious’s picture

Thank you! :D

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work

Should the doc header really say "Plugin implemention of..." - that seems like a conversion thing, every formatter will be a plugin once conversions are done.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

This is one of the last two formatter conversion patches (the other one is #1796316: Convert Link/URL widgets / formatters to plugin system) and *all* formatters from HEAD have this short description. Starting to diverge from it here would mean to be inconsistent with them so maybe we can make this change for all formatter and widget plugins in #1955678: Remove formatter and widgets legacy plugin?

xjm’s picture

swentel’s picture

Status: Reviewed & tested by the community » Postponed

We're going to postpone on #1735118: Convert Field API to CMI, that one is close, bear with us folks!

dawehner’s picture

Status: Postponed » Needs work

Now it's unblocked.

yched’s picture

Status: Needs work » Needs review
yched’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.34 KB
8.15 KB

Still green.
Attached patch just moves phpdocs to the new {@inheritdoc} standard.

Thus, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e9aa9c4 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

yched’s picture

Status: Closed (fixed) » Needs review
FileSize
2.39 KB

The patch inadvertently changed the formatter plugin ids:
'list_default' became 'options_list_default'
'list_key' became 'options_list_key'

We should get back to the previous names, or fields migrated from D7 will lose their formatters.
(also, the 'default_formatter' property in the field type definitions still refer to the old plugin ids)

dawehner’s picture

yched’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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