Problem/Motivation

Node types have a settings property which is meant to allow third parties to inject their per node type settings. However it's config schema and dependency implementation does not support this.

Proposed resolution

  • Fix schema
  • Implement calculate dependencies

Remaining tasks

Write patch, review and add tests.

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#29 2324121_29-interdiff.txt1011 bytesBerdir
#29 2324121_29.patch48.38 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,941 pass(es). View
#27 2324121_27-interdiff.txt773 bytesBerdir
#27 2324121_27.patch47.39 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,945 pass(es), 1 fail(s), and 0 exception(s). View
#25 2324121_25.patch46.64 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,838 pass(es), 328 fail(s), and 249 exception(s). View
#18 2324121_18-interdiff.txt1.01 KBBerdir
#18 2324121_18.patch47.05 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2324121_18.patch. Unable to apply patch. See the log in the details link for more information. View
#13 interdiff.txt2.24 KBslashrsm
#13 2324121_12.patch47.12 KBslashrsm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,666 pass(es). View
#11 node-type-third-party-2324121-11-interdiff.txt8.56 KBBerdir
#11 node-type-third-party-2324121-11.patch45.3 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,606 pass(es), 10 fail(s), and 0 exception(s). View
#9 node-type-third-party-2324121-9-interdiff.txt13.01 KBBerdir
#9 node-type-third-party-2324121-9.patch36.74 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,448 pass(es), 153 fail(s), and 54 exception(s). View
#7 node-type-third-party-2324121-7.patch27.41 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,534 pass(es), 27 fail(s), and 1 exception(s). View
#1 2324121.1.patch2.22 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,200 pass(es). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
2.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,200 pass(es). View

Initial crack at the patch.

xjm’s picture

Issue tags: +beta target

@effulgentsia pointed out that this is a schema addition, so no beta deadline. Makes sense as a beta target though.

alexpott’s picture

Status: Needs review » Postponed
swentel’s picture

Status: Postponed » Active

Go!

Berdir’s picture

Nice, will give this a try. This will overlap with #2226493: Apply formatters and widgets to Node base fields, which is cleaning up bogus node type options, would be awesome to get reviews/RTBC over there and get it out of the way.

Berdir’s picture

Status: Active » Needs review
FileSize
27.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,534 pass(es), 27 fail(s), and 1 exception(s). View

/me like ThirdPartySettingsTrait.

Based on the discussion in the other issue, I also killed of node.type.settings and moved the few remaining things to separate top-level properties, with better names and methods.

One thing I'm not sure about is the forced default value that you have to provide. It has the advantage that it immediately solves the problem that it is currently impossible to provide default node types with non-default menu_ui menu settings, as it forcefully overrides it, but it is a bit tedious to repeat and very unlike other configuration behavior (where the defaults stick and don't change when the coded default changes).

Will probably need some test for menu_ui default configuration/sync to make sure the schema stuff and so on is right, but a manual export test worked fine.

Status: Needs review » Needs work

The last submitted patch, 7: node-type-third-party-2324121-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
36.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,448 pass(es), 153 fail(s), and 54 exception(s). View
13.01 KB

Fixing those tests.

Status: Needs review » Needs work

The last submitted patch, 9: node-type-third-party-2324121-9.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
45.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,606 pass(es), 10 fail(s), and 0 exception(s). View
8.56 KB

Well, that is a disturbing amount of tests that create nodes with non-existing types.

We might want to add an existence check back, I'm surprised that actually still "works" but figured it makes sense to clean up those tests anyway, it's going to hurt us sooner or later.

Status: Needs review » Needs work

The last submitted patch, 11: node-type-third-party-2324121-11.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
47.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,666 pass(es). View
2.24 KB

Two more tests fixed.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Looks sensible!

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

This is still tagged with Needs tests, but I think we should be pretty good here.

+++ b/core/modules/node/node.module
@@ -635,11 +635,10 @@ function template_preprocess_node(&$variables) {
   // Display post information only on certain node types.
   // Avoid loading the entire node type config entity here that may not exist.
-  $node_type_config = \Drupal::config('node.type.' . $node->bundle());
+  $node_type = $node->type->entity;
   // Used by RDF to add attributes around the author and date submitted.
   $variables['author_attributes'] = new Attribute();
-  // Display submitted by default.
-  $variables['display_submitted'] = $node_type_config->isNew() || $node_type_config->get('settings.node.submitted');
+  $variables['display_submitted'] = $node_type->displaySubmitted();

I'm not sure if there was any profiling done on this and it's less a problem now with the render cache, but at least the comment above loading the node type needs to be updated. Setting back to needs work for that.

It might also make sense to wait before committing this until @alexpott is back, so he can confirm that this is in line with what he had in mind.

Another thing is $node->type->entity, we use that in a few places, not sure if we want to add a method for it. There is an issue somewhere that getType() is incorrectly named as it returns the ID of the type and not the type object. But the interesting/weird thing is that the ID property of node types is type, and getTypeType() would *not* make sense ;) But if we keep that, then I don't know how to name a method that returns the node type entity...

swentel’s picture

getNodeTypeType() !

alexpott’s picture

This patch looks great - I really the removal of the menu.entity.node configuration object.

  • +++ b/core/modules/node/src/Entity/NodeType.php
    @@ -9,6 +9,7 @@
     use Drupal\Component\Utility\NestedArray;
    

    This can be removed now.

  • I think we need to open a follow for the migration of menu_options_$type variables to the node type entity
  • I think we need a CR about the removal of menu_options_$type variables and their replacement with third party settings on the node type entity
Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
47.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2324121_18.patch. Unable to apply patch. See the log in the details link for more information. View
1.01 KB

- Removed the comment
- Removed the use
- #2333985: Migrate menu_default_node_menu setting. Fun stuff.

For change record, I'm thinking about what exactly we want, possibly two?

- One that just says that the specific menu settings are now third party settings of nodes and how to access them, possibly with a reference to the second.
- And a generic one that describes how to convert the 7.x node_type variable tricky to third party settings, as done here (with #entity_builders and stuff).

Makes sense?

benjy’s picture

+++ b/core/modules/book/config/install/node.type.book.yml
@@ -2,11 +2,8 @@ type: book
+new_revision: false
+display_submitted: true
+preview_mode: 1

I quite like this but I can't see anywhere in the issue where it was discussed to rename these? Surely this is a beta blocker if we're going to rename these?

Berdir’s picture

@benjy: I kind of agree. This is already tagged as beta target and a critical (which is a strange combination). Anyway, it should be ready except the change records.

Also, it's not just those renames. Moving the menu_ui configuration to third party settings is just as big a change.

Berdir’s picture

Done:

Generic change record: https://www.drupal.org/node/2334447
Menu-specific changes: https://www.drupal.org/node/2334449

there are existing issues for $node->type->entity/$node->getType(), I think we don't need to change this here.

Who wants to RTBC? :)

Berdir queued 18: 2324121_18.patch for re-testing.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

We've got test coverage, migration, change records, and I don't think performance is a concern here - let's get this done.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2324121_18.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
46.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,838 pass(es), 328 fail(s), and 249 exception(s). View

Re-roll, being optimistic.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2324121_25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
47.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,945 pass(es), 1 fail(s), and 0 exception(s). View
773 bytes

Clearly being too optimistic again.

Status: Needs review » Needs work

The last submitted patch, 27: 2324121_27.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
48.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,941 pass(es). View
1011 bytes

Fixing the new test.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

  • catch committed 4f978d9 on 8.0.x
    Issue #2324121 by Berdir, slashrsm, alexpott: Fixed NodeType's settings...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +D8 upgrade path

This is already tagged as beta target and a critical (which is a strange combination).

Just a note on this, it is a bit weird but I think it works:

Critical -> has to be done before release.

Beta target -> might have some impact on contrib modules and/or require data migration, however the change doesn't have enough impact to require actually blocking the beta.

So critical/beta target represents changes that we will do right up until RC, but are going to be more annoying (upgrade path needs writing, handful of contrib modules might get broken) the later they get done.

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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