Updated: Comment #17

Problem/Motivation

The ' Display author and date information.' and other node-type settings for content types are ignored when turned off
Steps to reproduce

  • Create a content type
  • Uncheck ' Display author and date information.'
  • Create a node of that type
  • View node in bartik theme
  • Result: Screenshot 2013-10-13 17.24.53.png

Proposed resolution

Ensure that node options and submitted variables are written as booleans to the node-type config entity.
Update node-type schema to flag these as booleans.
Update NodeType::getExportedProperties to correctly type these properties (ConfigEntityBase ignore config schemas).
Ensure config() isn't used to access node settings, use the config entity instead.

Remaining tasks

Reviews

User interface changes

None

API changes

None

None

Original report by @Mark Carver

"Display author and date information." checkbox on content types does not work. Introduced by #111715: Convert node/content types into configuration.

Steps to reproduce:

  1. Create new page and submit.
  2. Toggle "Display author and date information." on content type and watch it not change.

Need to test for this in the future.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Title: Submitted by always shows on node types » "Submitted by" always shows on node types
Status: Active » Needs review
FileSize
837 bytes

Here's the fix, we need tests.
Also, the comment above this line says to not load the config entity, but that would have prevented this bug...

Status: Needs review » Needs work

The last submitted patch, node-2053461-1.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.71 KB
1.43 KB

This is a bigger problem than I thought.
The default value is only applied on the form level, not the entity level.

Also, tests can create nodes of node types that don't exist.
Finally, NodeTestBase creates a page node type that doesn't match the expectations of the standard profile config.

markhalliwell’s picture

Issue tags: +Needs tests
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeCreationTest.php
@@ -56,6 +56,8 @@ function testNodeCreation() {
+    $this->assertNoText(t('Submitted by !username on !datetime', array('!username' => $this->loggedInUser->getUsername(), '!datetime' => format_date($node->getCreatedTime()))));

Can't really see with just the context lines, but are we also switching it on and testing that it shows up too?

markhalliwell’s picture

Issue tags: -Needs tests

wtf... tagging is screwed up. This wasn't even in the field.

Status: Needs review » Needs work

The last submitted patch, node-2053461-3-PASS.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Yep. Also need to fix those other tests.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.83 KB
6.72 KB

Fixed, expanded tests. Should be good now.

Status: Needs review » Needs work

The last submitted patch, node-2053461-8.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
803 bytes
6.78 KB

Last one.

swentel’s picture

aspilicious’s picture

Status: Needs review » Needs work

\Drupal:: in stead of Drupal::

swentel’s picture

Status: Needs work » Needs review
FileSize
5.86 KB

rerolled, lot of things have moved in since august.

Status: Needs review » Needs work

The last submitted patch, node-2053461-13.patch, failed testing.

larowlan’s picture

I vote we close this tackle as part of bigger issue in #2111091: Node type settings such as published state, promoted state, author information cannot be turned off, the issue also effects the status, promote and revision options. Thoughts?

tim.plunkett’s picture

This addresses those, just not in the issue title... But as long as the approaches are merged, I don't care. The two issues have completely different code ATM.
Let's chat on IRC later.

larowlan’s picture

Sure.
Fwiw I tested this manually before my 'vote', it fixes submission details, but not promoted our published so that's why I thought it had a narrower scope.

larowlan’s picture

Status: Needs work » Needs review
FileSize
12.39 KB
7.62 KB

Patch combines #13 and #2111091: Node type settings such as published state, promoted state, author information cannot be turned off which was closed as duplicate of this.
Issue summary updated to reflect wider scope.
Lets see how many fails left.

larowlan’s picture

Title: "Submitted by" always shows on node types » Node type settings such as published state, promoted state, create revision and author information cannot be turned off

New title

Status: Needs review » Needs work

The last submitted patch, node-post-settings-2053461.18.patch, failed testing.

larowlan’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
11.83 KB
876 bytes

This is surely major with the wider scope.

Status: Needs review » Needs work

The last submitted patch, node-post-settings-2053461.20.patch, failed testing.

larowlan’s picture

FileSize
2.44 KB

Missed a call to setDefaultRevision in EditFieldForm::init
I've made some new git alias toys so here's hoping they worked

From my .gitconfig

  interdiff = !git diff > ~/patches/interdiff.txt
  interdiff-stg = !git diff --staged > ~/patches/interdiff.txt
  genpatch = "!sh -c \"br=`git symbolic-ref HEAD|sed s#refs/heads/##`; git diff origin/8.x > ~/patches/\\${br}.$1.patch\""
  rebase8x = !git fetch origin && git rebase origin/8.x
  merge8x = !git fetch origin && git merge origin/8.x

Use like so - assuming branches are named {description}-{issuenumber}

# generate an interdiff based on current unstaged changes
git interdiff
# generate an interdiff based on current staged changes
git interdiff-stg
# rebase 8.x
git rebase8x
# merge 8.x
git merge8x
# generate a patch against 8.x with format {description}-{branch}.{comment}.patch
# here comment is 22
git genpatch 22
larowlan’s picture

Status: Needs work » Needs review
FileSize
13.6 KB

and how about a patch this time?

jibran’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/UrlTest.php
@@ -172,7 +172,7 @@ function testLinkRenderArrayText() {
+   * @papam $class

Typo

larowlan’s picture

Status: Needs work » Needs review
FileSize
13.1 KB
516 bytes

Fixes #25

tim.plunkett’s picture

  1. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -246,7 +246,10 @@ public function testUserWithPermission() {
    -      $this->container->get('config.factory')->get('node.type.article')->set('settings.node.options', array('status', 'revision'))->save();
    +      $this->container->get('config.factory')->get('node.type.article')->set('settings.node.options', array(
    +        'status' => TRUE,
    +        'revision' => TRUE,
    +      ))->save();
    

    Can we use entity_load('node_type', 'article') and set this properly?

  2. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -246,7 +246,10 @@ public function testUserWithPermission() {
    +      $this->container->get('config.factory')->get('node.type.article')->set('settings.node.options', array(
    
    +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -288,6 +288,10 @@ protected function drupalCreateNode(array $settings = array()) {
    +    if (!$this->container->get('entity.query')->get('node_type')->condition('type', $settings['type'])->execute()) {
    

    We're apparently to use \Drupal over $this->container in tests.

  3. +++ b/core/modules/node/lib/Drupal/node/Entity/NodeType.php
    @@ -211,4 +211,38 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
    +  public function getExportProperties() {
    +    $properties = parent::getExportProperties();
    +    // Ensure that submitted and options are booleans.
    +    $properties['settings']['node']['submitted'] = (bool) $properties['settings']['node']['submitted'];
    +    foreach ($properties['settings']['node']['options'] as $key => $value) {
    +       $properties['settings']['node']['options'][$key] = (bool) $value;
    +    }
    +    return $properties;
    +  }
    

    It really sucks that we have to do this.

  4. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
    @@ -43,7 +43,7 @@ protected function prepareEntity() {
    -          $node->$key = (int) in_array($key, $this->settings['options']);
    +          $node->$key = (int) (isset($this->settings['options'][$key]) ? $this->settings['options'][$key] : 0);
    

    This cast is suspicious. Can we remove the getExportProperties if we remove this? And make the default FALSE not 0

  5. +++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.php
    @@ -118,7 +112,7 @@ public function form(array $form, array &$form_state) {
    +      '#default_value' => MapArray::copyValuesToKeys(array_keys(array_filter($node_settings['options']))),
    

    O_o
    I guess this is needed? Not sure why.

  6. +++ b/core/modules/node/node.module
    @@ -702,8 +702,7 @@ function template_preprocess_node(&$variables) {
       // Avoid loading the entire node type config entity here.
    -  $submitted = \Drupal::config('node.type.' . $node->bundle())->get('settings.node.submitted') ?: TRUE;
    -  if ($submitted) {
    +  if (\Drupal::config('node.type.' . $node->bundle())->get('settings.node.submitted')) {
    

    In a follow-up, we should profile using the full entity here, because breaking our own rule of not using config() for entities sucks.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary

andypost’s picture

victor-shelepen’s picture

Hello. I see you are investigating? Could you make a small patch to make it workable? I see this problem lives too long. Let's use work around.

Thank you.

andypost’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch has test coverage so it needs re-roll and address #27

@likin You could try to re-roll it.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
8.43 KB
12.53 KB

Merge with following changes:
- Addressed #27 review
- converted standard profile config for node types
- added @todo to clean-up rfd config for page node type, somehow comment was removed, probably in node type cmi conversion

Status: Needs review » Needs work

The last submitted patch, 31: 2053461-node-options-31.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
14.61 KB

Patch adds fixes broken tests, that affected that "submitted" needs node-type entity config to display itself
Also #27.6 seems pointed to that, so the right fix would be to decide do we really need this this setting depend on node-type setting or use entity display somehow...

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -288,6 +288,10 @@ protected function drupalCreateNode(array $settings = array()) {
+    // If the specified node type does not exist, create it.
+    if (!$this->container->get('entity.query')->get('node_type')->condition('type', $settings['type'])->execute()) {
+      $this->drupalCreateContentType(array('type' => $settings['type'], 'name' => $this->randomString()));
+    }

Removal of this shows that node needs node type entity to properly display submitted variable. And only that!

PS: Can't reproduce failures in SimpleTestTest and ShortcutLinksTest locally

andypost’s picture

Clean-up patch

andypost’s picture

FileSize
5.33 KB
12.53 KB

right patch with interdiff

The last submitted patch, 34: 2053461-node-options-34.patch, failed testing.

victor-shelepen’s picture

Status: Needs review » Reviewed & tested by the community

In my case it works well.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

xjm’s picture

Component: node.module » node system

(Merging "node system" and "node.module" components for 8.x; disregard.)

Status: Fixed » Closed (fixed)

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