Re-instate field_extra_fields for 8.x [#2667692]

In 7.x we implemented hook_field_extra_fields() to allow the scheduler fieldset to be exposed in the content-type 'manage fields' tab, so that admins could adjust the position in the form.

This code was removed from 8.x in this commit. However, we still need this functionality in 8.x and it is still available - the hook has been renamed hook_entity_extra_field_info(). The core issue which changed it to entity API is #2225629: Move hook_field_extra_fields() to the entity API and here is the change record for extra field definitions and the hook_entity_extra_field_info() API page

Comment	File	Size	Author
#9	2667692-9.extra_field_info.patch	3.8 KB	jonathan1055
#9	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 32 pass
#7	2667692-7.extra_field_info_test_only.patch	3.94 KB	jonathan1055
#7	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 23 pass, 6 fail
#4	2667692-4.extra_field_info_test_only.patch	2.38 KB	jonathan1055
#4	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 22 pass, 7 fail
#2	2667692-1.extra_field_info.patch	1.56 KB	jonathan1055
#2	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 22 pass, 6 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

12 February 2016 at 13:37

jonathan1055 created an issue. See original summary.

Comment #2

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented 12 February 2016 at 13:45

Issue summary:	View changes
Status:	Active	» Needs review

File	Size
2667692-1.extra_field_info.patch	1.56 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 22 pass, 6 fail

Here is the function, converted to the new hook and updated for 8.x.
I think this should also have test coverage too, which I will work on, but for now here's the function, for review and comment.

Comment #3

12 February 2016 at 13:50

Status:

Needs review

» Needs work

The last submitted patch, 2: 2667692-1.extra_field_info.patch, failed testing.

Comment #4

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented 13 February 2016 at 19:31

Status:

Needs work

» Needs review

File	Size
2667692-4.extra_field_info_test_only.patch	2.38 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 22 pass, 7 fail

1 file was hidden/shown/deleted

File	Size
2667692-1.extra_field_info.patch	1.56 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 22 pass, 6 fail

I have added a second set of tests to FieldsDisplayTest to check that the weight item is shown on the form when it should be and not shown when it shouldn't. This patch contains just the changes to the test file, so there should be an extra failing class now.

Comment #5

13 February 2016 at 19:58

Status:

Needs review

» Needs work

The last submitted patch, 4: 2667692-4.extra_field_info_test_only.patch, failed testing.

Comment #6

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented 13 February 2016 at 20:26

As expected we get a new class fail

2	1	Scheduler.Drupal\scheduler\Tests\SchedulerFieldsDisplayTest
✓		- setUp
✓		- testFieldsDisplay
	✗	- testManageFormDisplay

fail: [Browser] Line 110 of modules/scheduler/src/Tests/SchedulerFieldsDisplayTest.php:
The scheduler settings weight entry is shown when the display mode is "fieldset"

Next I'll provide a patch with the tests and fixed code.

Comment #7

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented 13 February 2016 at 20:27

Status:

Needs work

» Needs review

File	Size
2667692-7.extra_field_info_test_only.patch	3.94 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 23 pass, 6 fail

Patch with new test + fixed .module

Comment #8

13 February 2016 at 20:39

Status:

Needs review

» Needs work

The last submitted patch, 7: 2667692-7.extra_field_info_test_only.patch, failed testing.

Comment #9

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented 10 March 2016 at 15:34

Status:

Needs work

» Needs review

File	Size
2667692-9.extra_field_info.patch	3.8 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 32 pass

2 files were hidden/shown/deleted

File	Size
2667692-4.extra_field_info_test_only.patch	2.38 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 22 pass, 7 fail
2667692-7.extra_field_info_test_only.patch	3.94 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.1 23 pass, 6 fail

I realised that there is no need to only allow the weight entry for 'fieldset'. So

if (($publishing_enabled || $unpublishing_enabled) && $fields_display_mode == 'fieldset') {

simply becomes

if ($publishing_enabled || $unpublishing_enabled) {

If this restriction is removed then the weight setting works for vertical tabs too, which is better. New patch, including tests.

Comment #10

pfrenssen

Sofia

CreditAttribution: pfrenssen commented 10 March 2016 at 20:17

Status:

Needs review

» Reviewed & tested by the community

I don't remember why I deleted this, apparently I thought this was no longer used since we are now using base fields. I possibly thought all the relevant code was already factored out of it and this was just the wrapper. But yes we obviously still need it.

Thanks for adding test coverage, this will ensure I will not mess with it any more in the future :)

Also very very nice to see a fully green test. Amazing work!!

If this restriction is removed then the weight setting works for vertical tabs too, which is better.

Good idea!

+++ b/scheduler.module
@@ -458,6 +458,30 @@ function _scheduler_scheduler_api(NodeInterface $node, $action) {
+function scheduler_entity_extra_field_info() {
+  // Expose the Scheduler group on the 'Manage Form Display' tab when editing a
+  // content type. This allows admins to adjust the weight of the group, and it
+  // works for vertical tabs and separate fieldsets.

Thanks for adding this documentation, that clarifies why we need it.

+++ b/scheduler.module
@@ -458,6 +458,30 @@ function _scheduler_scheduler_api(NodeInterface $node, $action) {
+      $fields['node'][$type->get('type')]['form']['scheduler_settings'] = array(

Still using the old array() syntax. This is OK since it is consistent with the remainder of the module file. We should maybe do a cleanup to convert all of these when we are closer to release.

```
+++ b/src/Tests/SchedulerFieldsDisplayTest.php
@@ -17,6 +17,12 @@ use Drupal\node\Entity\NodeType;
   /**
+   * SchedulerTestBase loads the standard modules.
+   * Additional module field_ui is required for the 'manage form display' test.
+   */
+  public static $modules = ['field_ui'];
```
This surprised me. This should override the static from SchedulerTestBase, so logically the Node module and Scheduler itself should no longer be enabled now. But since the tests are green this seems to work.

I looked into it and this is handled by WebTestBase::installModulesFromClassProperty(). It loops over all parent classes and merges the properties together. Nice trick, I didn't know this.

+++ b/src/Tests/SchedulerFieldsDisplayTest.php
@@ -75,4 +81,31 @@ class SchedulerFieldsDisplayTest extends SchedulerTestBase {
+  /**
+   * Tests the settings entry in the content type form display.
+   * This test covers scheduler_entity_extra_field_info().
+   */

There is the @covers keyword now, but I'm not sure if this applies to WebTestBase as well. It does apply to PHPUnit based tests. It's fine to leave it for now.

Looking good, RTBC!

Comment #11

10 March 2016 at 20:19

pfrenssen committed 00ea060 on 8.x-1.x authored by jonathan1055

Issue #2667692 by jonathan1055: Re-instate field_extra_fields for 8.x.

Comment #12

pfrenssen

Sofia

CreditAttribution: pfrenssen commented 10 March 2016 at 20:19

Assigned:	jonathan1055	» Unassigned
Status:	Reviewed & tested by the community	» Fixed

Committed to 8.x-1.x. Thanks!

Comment #13

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented 11 March 2016 at 08:34

Hi Pieter,
Thanks for all the review info above.
2. Yes we could do an array syntax cleanup. Let's pick a time when we have only a few patches waiting review, as it is bound to cause many re-rolls.
3. SchedulerTestBase loads the standard modules ... I discovered this by chance, checked it, then I cleaned up the other test files, removing the unnecessary lines. I added the comment specifically to say way, and share the knowledge. Thanks for looking deeper into the reason.
4. I did not know about @covers and will use it in future test coding.

Yes, it is very good to see the full set of green test passes. In addition to being "a good thing" in itself, it also makes our development/review/retest process cleaner and easier, and the issue status properly reflects the state of the submitted patch .