Updated: Comment #0

See Issue summary for #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title)

This issue is about the following base fields of the Node entity type (basically everything except the title):

  1. author (uid)
  2. publication date (created)
  3. language (langcode)
  4. published (status)
  5. stick at top of lists (sticky)
  6. revision log message (log)

This includes the addition of new widgets / formatters:

  1. author (uid): AuthorFormatter (generic, for EntityOwnerInterface entities) + RouteBasedAutocompleteWidget (generic, to not depend on entity_reference.module) + AuthorAutocompleteWidget (extends the previous one with author-specific UX)
  2. publication date (created): TimestampFormatter (generic) + TimestampWidget (generic)
  3. published (status) & stick at top of lists (sticky): BooleanWidget (generic)
  4. revision log message (log)

Only the language (langcode) field (and its corresponding LanguageWidget) will not be done in this issue, for that we have #2230637: Create a Language field widget and the related formatter.

And for datetime.module's "enhancing override" to switch the default text field widget to a fancy datetime widget now has a DateTimeTimestampWidget that can be used for any TimestampItem — this also cleans up datetime.module.

Also removes node_field_extra_fields()! :)

And last but not least, this finally makes the node author and date in-place editable! Or well… almost. In-place editing the author fails, for reasons that swentel and I don't yet understand. Hopefully yched, fago or amateescu can shed some light on that.

Remaining tasks

  1. Fix test failures.

User interface changes

None.

API changes

None. The base fields above are rendered using widgets and formatters, and are no longer exposed as an "extra field".

CommentFileSizeAuthor
#205 2226493-205-interdiff.txt589 bytesBerdir
#205 2226493-205.patch66.06 KBBerdir
#201 interdiff.txt2.51 KBjoelpittet
#201 2226493-200.patch66.06 KBjoelpittet
#193 2226493-193-interdiff.txt2.39 KBBerdir
#193 2226493-193.patch66.03 KBBerdir
#190 2226493-190-interdiff.txt2.35 KBBerdir
#190 2226493-190.patch66.4 KBBerdir
#188 2226493-188-interdiff.txt653 bytesBerdir
#188 2226493-188.patch66.52 KBBerdir
#1 node_base_fields_formatters_widgets-2226493-1.patch38.68 KBWim Leers
#4 node_base_fields_formatters_widgets-2226493-4.patch38.74 KBWim Leers
#6 node_base_fields_formatters_widgets-2226493-6.patch38.88 KBWim Leers
#8 node_base_fields_formatters_widgets-2226493-8.patch38.93 KBWim Leers
#13 2226493-8-13-interdiff.txt10 KBmr.baileys
#13 2226493-13-node-base-fields.patch62.84 KBmr.baileys
#16 2226493-13-16-interdiff.txt11.34 KBmr.baileys
#16 2226493-16-node-base-fields.patch48.61 KBmr.baileys
#18 2226493-18-node-base-fields.patch49.39 KBmr.baileys
#18 2226493-interdiff.txt6.94 KBmr.baileys
#23 2226493-23-node-base-fields.patch50.54 KBmr.baileys
#23 2226493-interdiff.txt3.58 KBmr.baileys
#27 2226493-27-node-base-fields.patch41.91 KBBerdir
#27 2226493-27-node-base-fields-interdiff.txt10.38 KBBerdir
#29 2226493-29-node-base-fields.patch42.96 KBBerdir
#29 2226493-29-node-base-fields-interdiff.txt2.05 KBBerdir
#31 2226493-interdiff.txt4.72 KBmr.baileys
#31 2226493-31-node-base-fields.patch47.68 KBmr.baileys
#33 2226493-interdiff.txt2.17 KBmr.baileys
#33 2226493-33-node-base-fields.patch46.96 KBmr.baileys
#36 2226493-36-node-base-fields.patch46.96 KBscor
#37 2226493-37-node-base-fields.patch56.52 KBscor
#37 interdiff.txt9.57 KBscor
#41 2226493-41-node-base-fields.patch55.62 KBWim Leers
#41 interdiff.txt1.33 KBWim Leers
#48 2226493-48-node-base-fields.patch54.38 KBmr.baileys
#48 interdiff.txt7.03 KBmr.baileys
#51 node_base_fiels-2226493-51.patch56.44 KBWim Leers
#51 interdiff.txt3.27 KBWim Leers
#53 node_base_fiels-2226493-53.patch55.99 KBWim Leers
#53 interdiff.txt5.49 KBWim Leers
#55 node_base_fields-2226493-55.patch59.02 KBWim Leers
#55 interdiff.txt3.09 KBWim Leers
#57 node_base_fields-2226493-57.patch59.59 KBWim Leers
#57 interdiff.txt5.04 KBWim Leers
#58 node_base_fields-2226493-58.patch58.64 KBWim Leers
#62 node_base_fields-2226493-62.patch58.56 KBWim Leers
#64 node_base_fields-2226493-64.patch58.49 KBWim Leers
#64 interdiff.txt1.32 KBWim Leers
#66 node_base_fields-2226493-66.patch58.43 KBWim Leers
#68 node_base_fields-2226493-68.patch61.31 KBWim Leers
#68 interdiff.txt2.99 KBWim Leers
#71 node_base_fields-2226493-71.patch61.32 KBWim Leers
#75 node_base_fields-2226493-75.patch59.62 KBBerdir
#77 node_base_fields-2226493-77.patch58.97 KBBerdir
#77 node_base_fields-2226493-77-interdiff.txt8.46 KBBerdir
#79 node_base_fields-2226493-79.patch59.73 KBBerdir
#79 node_base_fields-2226493-79-interdiff.txt2.35 KBBerdir
#81 node_base_fields-2226493-81.patch60.29 KBBerdir
#81 node_base_fields-2226493-81-interdiff.txt6.41 KBBerdir
#83 node_base_fields-2226493-83.patch62.09 KBBerdir
#83 node_base_fields-2226493-83-interdiff.txt3.25 KBBerdir
#86 node_base_fields-2226493-86.patch62.92 KBBerdir
#86 node_base_fields-2226493-86-interdiff.txt2.47 KBBerdir
#88 node_base_fields-2226493-88.patch64.73 KBBerdir
#88 node_base_fields-2226493-88-interdiff.txt4.25 KBBerdir
#89 node_base_fields-2226493-89.patch64.82 KBBerdir
#89 node_base_fields-2226493-89.patch63.27 KBBerdir
#89 node_base_fields-2226493-89-interdiff.txt2.16 KBBerdir
#95 2226493-comment-base-98.patch9.14 KBandypost
#96 interdiff.txt1007 bytesandypost
#96 2226493-comment-base-99.patch9.11 KBandypost
#100 node_base_fields-2226493-100.patch67.36 KBWim Leers
#106 node_base_fields-2226493-106.patch63.14 KBBerdir
#106 node_base_fields-2226493-106-interdiff.txt1.46 KBBerdir
#108 node_base_fields-2226493-108.patch59.64 KBBerdir
#108 node_base_fields-2226493-108-interdiff.txt8.14 KBBerdir
#114 node_base_fields-2226493-114.patch57.65 KBm1r1k
#116 node_base_fields-2226493-116.patch57.25 KBm1r1k
#116 interdiff.txt3.9 KBm1r1k
#118 node_base_fields-2226493-118.patch57.67 KBm1r1k
#118 interdiff.txt1.25 KBm1r1k
#120 node_base_fields-2226493-120.patch57.73 KBm1r1k
#120 interdiff.txt1.11 KBm1r1k
#123 interdiff.txt5.16 KBm1r1k
#123 node-base-fields-2226493-123.patch59.13 KBm1r1k
#125 interdiff.txt2.69 KBm1r1k
#125 node-base-fields-2226493-125.patch60.96 KBm1r1k
#127 interdiff.txt1.83 KBm1r1k
#127 node-base-fields-2226493-127.patch61.32 KBm1r1k
#129 interdiff.txt6.15 KBandypost
#129 2226493-node-base-129.patch61.92 KBandypost
#131 interdiff.txt53.52 KBm1r1k
#131 2226493-node-base-131.patch62.62 KBm1r1k
#133 interdiff.txt1.59 KBm1r1k
#133 2226493-node-base-132.patch62.5 KBm1r1k
#135 2226493-node-base-135.patch62.36 KBcbr
#139 2226493-node-base-139.patch62.63 KBcbr
#139 2226493-node-base-139-interdiff.txt3.29 KBcbr
#143 interdiff.txt2.61 KBandypost
#143 2226493-143.patch62.8 KBandypost
#149 2226493-149.patch63.19 KBBerdir
#149 2226493-149-interdiff.txt6.08 KBBerdir
#150 2226493-150.patch63.19 KBBerdir
#150 2226493-150-interdiff.txt565 bytesBerdir
#154 2226493-154.patch62.89 KBBerdir
#154 2226493-154-interdiff.txt1.88 KBBerdir
#156 2226493-156.patch62.89 KBBerdir
#159 2226493-159.patch63.12 KBBerdir
#159 2226493-159-interdiff.txt970 bytesBerdir
#166 2226493-166.patch67.98 KBBerdir
#166 2226493-166-interdiff.txt5.42 KBBerdir
#168 2226493-168.patch65.09 KBBerdir
#168 2226493-168-interdiff.txt8.72 KBBerdir
#171 2226493-171.patch62.85 KBBerdir
#171 2226493-171-interdiff.txt12.75 KBBerdir
#173 2226493-173.patch63.58 KBBerdir
#173 2226493-173-interdiff.txt3.57 KBBerdir
#179 2226493-176.patch63.86 KBBerdir
#179 2226493-176-interdiff.txt3.33 KBBerdir
#185 2226493-185.patch66.27 KBBerdir
#185 2226493-185-interdiff.txt5.83 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers’s picture

Status: Active » Needs review
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
38.74 KB

Lots of conflicts in rebasing. I hope I got it all right.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
38.88 KB

Oops.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
38.93 KB

This is turning into a real clusterfuck. I fixed one annotation incorrectly.

I still can't install it locally, but I don't get a useful error, so… testbot to the rescue!

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/TimestampWidget.php
    @@ -0,0 +1,73 @@
    +   * @todo Convert to massageFormValues() override once that actually works.
    

    Would be useful to have an issue or something here...

  2. +++ b/core/modules/datetime/datetime.module
    @@ -986,23 +986,13 @@ function datetime_range_years($string, $date = NULL) {
    +function datetime_entity_base_field_info_alter(&$fields, \Drupal\Core\Entity\EntityTypeInterface $entity_type) {
    

    This is awesome :)

  3. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -380,23 +386,48 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setCardinality(1)
    

    This should be the default for base fields.

  4. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -429,7 +469,18 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
         $fields['log'] = FieldDefinition::create('string')
           ->setLabel(t('Log'))
           ->setDescription(t('The log entry explaining the changes in this revision.'))
    -      ->setRevisionable(TRUE);
    +      ->setLabel(t('Revision log message'))
    +      ->setCardinality(1)
    +      ->setDescription(t('Briefly describe the changes you have made.'))
    +      ->setRevisionable(TRUE)
    +      ->setDisplayOptions('form', array(
    +        'type' => 'text_textarea',
    +        'weight' => 0,
    +        'settings' => array(
    +          'rows' => 4,
    +        ),
    +      ))
    +      ->setDisplayConfigurable('form', TRUE);
    

    Interesting, should the revision log maybe be a string_long? Haven't checked.

  5. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -429,7 +469,18 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setDescription(t('Briefly describe the changes you have made.'))
    

    Just copied I guess, but is that even valid english? :)

  6. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -444,6 +495,22 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
    +      foreach (node_type_get_types() as $bundle) {
    +        $configuration = language_get_default_configuration('node', $bundle->type);
    +        if ($configuration['language_show']) {
    +          $fields['langcode'] = clone $base_field_definitions['langcode'];
    +          $fields['langcode']->setDisplayOptions('form', array(
    +            'type' => 'language',
    +            'weight' => 0,
    +          ));
    +        }
    +      }
    

    this is called for a specific bundle, you should just use that and not loop over all of them.

    Should also make it configurable?

  7. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
    @@ -310,17 +285,12 @@ public function validate(array $form, array &$form_state) {
     
         // Validate the "authored by" field.
    -    if (!empty($form_state['values']['uid']) && !($account = user_load_by_name($form_state['values']['uid']))) {
    -      // The use of empty() is mandatory in the context of usernames
    -      // as the empty string denotes the anonymous user. In case we
    -      // are dealing with an anonymous user we set the user ID to 0.
    -      $this->setFormError('uid', $form_state, $this->t('The username %name does not exist.', array('%name' => $form_state['values']['uid'])));
    +    if (!is_numeric($form_state['values']['uid'][0]['value'])) {
    +      $this->setFormError('uid', $form_state, $this->t('The username %name does not exist.', array('%name' => $form_state['values']['uid'][0]['value'])));
         }
    

    Now that it's a widget and the field attach form stuff landed, this might work based on the validation API now?

  8. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
    @@ -310,17 +285,12 @@ public function validate(array $form, array &$form_state) {
         // Validate the "authored on" field.
    -    // The date element contains the date object.
    -    $date = $node->date instanceof DrupalDateTime ? $node->date : new DrupalDateTime($node->date);
    -    if ($date->hasErrors()) {
    +    if ($form_state['values']['created'][0]['value'] < 0) {
           $this->setFormError('date', $form_state, $this->t('You have to specify a valid date.'));
         }
    

    Same here.

  9. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
    @@ -418,17 +388,11 @@ public function unpublish(array $form, array &$form_state) {
    +    $entity->setOwnerId($form_state['values']['uid'][0]['value']);
    

    Should also not be needed anymore if the widget is working correctly.

  10. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
    @@ -418,17 +388,11 @@ public function unpublish(array $form, array &$form_state) {
    +
    +    if ($form_state['values']['created'][0]['value'] >= 0) {
    +      $entity->setCreatedTime($form_state['values']['created'][0]['value']);
         }
         else {
           $entity->setCreatedTime(REQUEST_TIME);
    

    Would be nice if this could be a part of the widget logic too, but might be node specific, not sure.

Berdir’s picture

Also #2226063: Merge ListBooleanItem from options module into BooleanItem is merging the boolean field and adding widget/formatter for it too.

mr.baileys’s picture

Assigned: Unassigned » mr.baileys

Assigning to myself to try and resolve the test failures during the sprint.

mr.baileys’s picture

Assigned: mr.baileys » Unassigned
Status: Needs work » Needs review
FileSize
10 KB
62.84 KB

I was able to fix some of the failing tests, but then ran out of time. Attached is the result of my work in case anyone wants to pick up were I left. If not I might be able to continue later this week.

Settings to Needs Review to get a new test report.

mr.baileys’s picture

Assigned: Unassigned » mr.baileys

Working on this together with Wim Leers.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
11.34 KB
48.61 KB

work-in-progress...

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
49.39 KB
6.94 KB

Another test failure fixed, this time with *a lot* of help from Wim Leers & fago.

andypost’s picture

Overall great, the main issue here that Core and modules responsibility mixed.

Boolean field has own issue #2226063: Merge ListBooleanItem from options module into BooleanItem

Entity reference formatter could move to Core 2 default ER module (label and string) but autocomplete widget require route, but Core can't use routes.
Lack of the route support leads to really bad DX

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/AuthorFormatter.php
    @@ -0,0 +1,47 @@
    +class AuthorFormatter extends FormatterBase {
    

    Maybe better to use here a ER label formatter by moving it to Core\Field\...?

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/AuthorFormatter.php
    @@ -0,0 +1,47 @@
    +      /** @var $referenced_user \Drupal\user\UserInterface */
    +      if ($referenced_user = $item->entity) {
    +        $elements[$delta] = array(
    +          '#theme' => 'username',
    +          '#account' => $referenced_user,
    

    Any reason in this variable and assignment?

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -0,0 +1,40 @@
    + *   field_types = {
    + *     "timestamp",
    + *     "created",
    

    why not add "changed" here?

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
    @@ -18,7 +18,8 @@
      *   label = @Translation("Boolean"),
    ...
    + *   no_ui = TRUE,
    + *   default_widget = "boolean",
    

    Related issue #2226063: Merge ListBooleanItem from options module into BooleanItem
    Trying to de-duplicate boolean fields already... Maybe join efforts?

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AuthorAutocompleteWidget.php
    @@ -0,0 +1,77 @@
    + *   id = "author_autocomplete",
    ...
    + *     "route_name" = "",
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/RouteBasedAutocompleteWidget.php
    @@ -0,0 +1,42 @@
    + *   settings = {
    + *     "route_name" = "",
    ...
    +      '#autocomplete_route_name' => $this->getSetting('route_name'),
    
    +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -381,13 +381,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
         $fields['uid'] = FieldDefinition::create('entity_reference')
    ...
    +        'settings' => array(
    +          'route_name' => 'user.autocomplete',
    

    DX--
    So each time I add user_atocomplete I need to remember the route?

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AuthorAutocompleteWidget.php
    @@ -0,0 +1,77 @@
    + *   settings = {
    

    No more after #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageWidget.php
    @@ -0,0 +1,45 @@
    +      '#type' => 'language_select',
    

    defined in language module, Core should not depend on it!

Wim Leers’s picture

We should also be able to remove this — at last :)

    // The node 'submitted' info is not rendered in a standard way (renderable
    // array) so we have to add a cache tag manually.
    $build['#cache']['tags']['user'][] = $entity->getOwnerId();
Wim Leers’s picture

And one more : we should be able to remove this hack too:

/**
 * Implements hook_entity_form_display_alter().
 */
function node_entity_form_display_alter(EntityFormDisplayInterface $form_display, $context) {
  if ($context['entity_type'] == 'node') {
    $node_type = node_type_load($context['bundle']);
    // @todo Reconsider when per-bundle overrides of field definitions are
    //   possible - https://drupal.org/node/2114707.
    if (!$node_type->has_title) {
      $form_display->removeComponent('title');
    }
  }
}

#2114707: Allow per-bundle overrides of field definitions landed, so we can do this in Node::bundleFieldDefinitions() :)

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
50.54 KB
3.58 KB
  • Fixed the failures on ConfigImportRecreateTest (thanks to help from swentel). The node base fields like sticky, promote, etc should not be configurable since Seven is rearranging the node interface anyway, so any custom re-ordering of those fields will be ignored anyway.
  • Removed the code mentioned in #21 and #22

Still need to fix the remaining test failures and adress the remarks from #20.

Wim Leers’s picture

Issue summary: View changes

After a discussion with plach, it was decided that it's not feasible to create a LanguageWidget in this issue, because there is too much code to untangle, which would significantly expand the scope of this issue.

To handle that, I've opened #2230637: Create a Language field widget and the related formatter.

The next reroll of this patch should no longer include LanguageWidget.

scor’s picture

Issue tags: +RDF code sprint

+1. If we have author and created date as generic field formatters, we could remove some the custom code from rdf.module that currently deals with adding RDFa markup to author and date in nodes and comments output (aka submitted by). I believe this issue would be easily solved too: #2047095: Remove $submitted from node templates.

Berdir’s picture

Status: Needs work » Needs review
FileSize
41.91 KB
10.38 KB

Re-roll and reverted the langcode changes.

Berdir’s picture

Status: Needs work » Needs review
FileSize
42.96 KB
2.05 KB

Fixed some tests, but the fixes are pretty ugly. Not sure why the author widget is even used for EntityTest although the fix makes sense, but the translation controllers are ugh.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
47.68 KB

This should address a lot of the remaining test failures.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
46.96 KB

I think I got it down to a single failing test.

Berdir’s picture

Assigned: mr.baileys » scor

Asked @scor about how to properly deal with this in rdf.module, it's something that's existing in HEAD but is not visible as the created field doesn't have a view display component so the field mapping there is ignored.

Two things that I can thing of:
- Add a different callback function that is able to deal with an array('value' => 345365) structure for those fields
- Allow to define mappings for field properties instead of fields, so that you can define created.value instead of created.

@scor also had a different idea I think, assigning to him.

scor’s picture

reroll.

scor’s picture

Status: Needs work » Needs review
FileSize
56.52 KB
9.57 KB

I decided to take the approach to use a different callback function that is able to deal with an array as value.

andypost’s picture

+++ b/core/modules/rdf/rdf.module
@@ -304,7 +304,7 @@ function rdf_preprocess_node(&$variables) {
-    $date_attributes = rdf_rdfa_attributes($created_mapping, $variables['node']->getCreatedTime());
+    $date_attributes = rdf_rdfa_attributes($created_mapping, $variables['node']->get('created')[0]->getValue());

not sure this change is needed, get(created)->value should return the same

scor’s picture

@andypost nope, $variables['node']->getCreatedTime() returns a UNIX timestamp string and $variables['node']->get('created')[0]->getValue() returns an array('value' => {timestamp}).

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
55.62 KB
1.33 KB

I cannot reproduce the fails in Drupal\edit\Tests\EditLoadingTest locally.

Fixed the Drupal\node\Tests\NodeTranslationUITest failures.

Wim Leers’s picture

I still can't reproduce those last 3 failures (and 2 related exceptions) in EditLoadingTest. I wonder if it's because I'm on PHP 5.5…

Wim Leers’s picture

aspilicious’s picture

--- a/core/modules/text/lib/Drupal/text/Plugin/Field/FieldWidget/TextareaWidget.php
+++ b/core/modules/text/lib/Drupal/text/Plugin/Field/FieldWidget/TextareaWidget.php
@@ -18,7 +18,12 @@
  *   id = "text_textarea",
  *   label = @Translation("Text area (multiple rows)"),
  *   field_types = {
- *     "text_long"
+ *     "text_long",
+ *     "string_long",
+ *   },
+ *   settings = {
+ *     "rows" = "5",
+ *     "placeholder" = ""

I can be wrong but didn't we move the settings out of annotations? Or is that issue not in yet...

yched’s picture

Issue summary: View changes
mr.baileys’s picture

Assigned: scor » mr.baileys

I have some time today to re-roll this patch and attempt to fix the remaining test failures.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
54.38 KB
7.03 KB

Re-rolled to keep up with HEAD.

I can be wrong but didn't we move the settings out of annotations?

You're correct, somewhere between patches, #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition got committed. I updated this patch so settings are moved out of the annotations and into defaultSettings().

QuickEditIntegrationLoadingTest was now failing with array to string conversion warnings in theme_field__node__*, fixed by wrapping $variables['attributes'] in new Attribute().

PageCacheTagsIntegrationTest was failing again. I have removed the "user_view:1" from the list of expected tags (this was added earlier in this thread, but since the 'submitted'-text is still hard-wired rather than rendered, this tag is not present). Not really sure about this one though, would be great to get input from someone who knows more about cache tags than I do.

webchick’s picture

Priority: Critical » Major

As much as I would still love to see this happen, since it's very silly as an end user to only be able to in-place edit some things but not others on a node, the core maintainers discussed this today on a call going through all of the outstanding criticals, and we are very hard-pressed to make this a release blocker atm. It doesn't represent a regression from Drupal 7, and while this issue was originally marked critical as a blocker to hook_help() (another critical), but that's a false dependency; the docs can still be written and then just amended later when/if this makes it into core. (Hopefully, "when." :))

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
56.44 KB
3.27 KB

Rerolled.

Now has Twig templates for the uid and created fields on nodes, just like title has since #2216437: Entity labels are not in-place editable on "full entity page" (prime example: node title).

The comment settings are now misplaced; they no longer end up having vertical tabs, and hence don't end up in Seven's "secondary form region". They work just fine, but are placed incorrectly. Currently debugging that, but in the mean time testbot can have a look at this :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
55.99 KB
5.49 KB

PageCacheTagsIntegrationTest was failing again. I have removed the "user_view:1" from the list of expected tags (this was added earlier in this thread, but since the 'submitted'-text is still hard-wired rather than rendered, this tag is not present). Not really sure about this one though, would be great to get input from someone who knows more about cache tags than I do.

This is correct.
Since #2099131: Use #pre_render pattern for entity render caching, it's now the responsibility of formatters to declare cache tags, which explains this difference over time.
(Technically, the user_view:1 tag should appear, but it cannot, because user_view($node->getOwner(), 'compact') is being invoked in the theme layer (in template_preprocess_node()), which is the wrong place to be rendering entities — it makes it impossible to bubble up cache tags. Fixing that is definitely out of scope for this issue.)

I was finally able to reproduce the failure in Quick Edit (the one that was responsible for the last few test failures back when this patch was almost ready — see #43), and fixed that!

We should now be at zero failures.


That leaves us with two todos:

  1. I can be wrong but didn't we move the settings out of annotations? Or is that issue not in yet...
  2. The comment settings are now misplaced; they no longer end up having vertical tabs, and hence don't end up in Seven's "secondary form region". They work just fine, but are placed incorrectly.
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
59.02 KB
3.09 KB

Should be green.

Berdir’s picture

  1. +++ b/core/modules/node/lib/Drupal/node/NodeForm.php
    @@ -92,7 +90,8 @@ public function form(array $form, array &$form_state) {
           '#languages' => Language::STATE_ALL,
           '#access' => isset($language_configuration['language_show']) && $language_configuration['language_show'],
    -    );
    +     );
    +
     
    

    unnecessary and wrong (indentation) left-over...

  2. +++ b/core/modules/node/lib/Drupal/node/NodeTranslationHandler.php
    @@ -80,8 +80,9 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
           // $form['content_translation']['name'] is the equivalent field
           // for translation author uid.
    -      $translation['name'] = $form_state['values']['uid'];
    -      $translation['created'] = $form_state['values']['created'];
    +      $account = user_load($form_state['values']['uid'][0]['target_id']);
    +      $translation['name'] = $account ? $account->getUsername() : '';
    +      $translation['created'] = format_date($form_state['values']['created'][0]['value'], 'custom', 'Y-m-d H:i:s O');
         }
         parent::entityFormEntityBuild($entity_type, $entity, $form, $form_state);
    

    This override stuff is really crazy, but I don't know how else to clean it up...

This looks close.

You might notice that we're adding a lot more code than it removes, but most of it is in more or less re-usable widgets that other entity forms will be able to use as well.

Wim Leers’s picture

FileSize
59.59 KB
5.04 KB

Thanks for the review!

  1. Fixed #56.1. (unnecessary and wrong (indentation) left-over...)
  2. Fixed #45. Fixed (I can be wrong but didn't we move the settings out of annotations? Or is that issue not in yet...)
  3. Fixed my own bug report The comment settings are now misplaced; they no longer end up having vertical tabs, and hence don't end up in Seven's "secondary form region". They work just fine, but are placed incorrectly.

With that, I know of no more remaining issues! Finally! :)

Wim Leers’s picture

FileSize
58.64 KB

Rerolled now that the PSR-4 patch has landed.

mr.baileys’s picture

Assigned: mr.baileys » Unassigned
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks like all outstanding issues have been addressed.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
58.56 KB

Straight reroll; #2276183: Date intl support is broken, remove it broke this in a minor way.

Back to RTBC per #60.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
58.49 KB
1.32 KB

#62 included one remaining call to datetime_default_format_type(), which was removed in #2276183: Date intl support is broken, remove it. Now fixed.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
58.43 KB

Forgot to pull for #64. Straight reroll.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
61.31 KB
2.99 KB

Sigh. Deep sigh.

/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldWidget/DateTimeTimestampWidget.php
->
/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeTimestampWidget.php

(Sorry for all the noise.)

This fixed most of the fails, but there was still a fail in EntityTranslationFormTest.php, which I also fixed.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC per #60.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts
FileSize
61.32 KB

#2247211: Autocomplete widget doesn't work on entity labels without a " (entity_id)" suffix broke two hunks of this patch. Straight reroll.

I think this patch is sufficiently important & large to be marked to avoid commit conflicts.

andypost’s picture

I still think that one needs follow-up to add tests for newly introduced widgets and formatters

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

Overall the patch looks great, lots of stuff to love. Thanks for all the work on this, impressive!

I have a bunch of remarks. Most of them are related to injecting things and allowing for better unit testing. You can safely ignore those, if you don't want to bother with this now, we can also handle that in a follow-up. There are a few items, that I am marking this needs work for, though. Will comment in the next comment which items those are (because I can't preview my comment...).

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/AuthorFormatter.php
    @@ -0,0 +1,50 @@
    + * Contains \Drupal\Core\Field\Plugin\Field\FieldFormatter\AuthorFormatter.
    ...
    +          '#theme' => 'username',
    

    theme_username() is in User module, so AuthorFormatter should be as well.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -0,0 +1,40 @@
    +      $elements[$delta] = array('#markup' => format_date($item->value));
    

    Let's not use the deprecated format_date() but instead let's use \Drupal::service('date')->format(). Instead of calling that directly, though, please use a protected function date() to allow for unit testing.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AuthorAutocompleteWidget.php
    @@ -0,0 +1,74 @@
    + * Contains \Drupal\Core\Field\Plugin\Field\FieldWidget\RouteBasedAutocompleteWidget.
    ...
    +    $user_config = \Drupal::config('user.settings');
    ...
    +      $account = user_load_by_name($value);
    

    Again, this widget should belong to User module.

    Also it would be great to put \Drupal::config() into a protected function config(). Similarly, instead of using user_load_by_name() you could add a protected entityManager() or event userStorage() and use loadByProperties() or alternatively a entityQuery() to perform the entity query manually.

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AuthorAutocompleteWidget.php
    @@ -0,0 +1,74 @@
    +    $element['target_id']['#default_value'] = $entity->getOwner()->isAuthenticated() ? $entity->getOwner()->getUsername() : '';
    

    This should include a check for $entity instanceof EntityOwnerInterface and throw an exception if it is FALSE.

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/RouteBasedAutocompleteWidget.php
    @@ -0,0 +1,48 @@
    + * @FieldWidget(
    + *   id = "route_based_autocomplete",
    + *   label = @Translation("Entity reference autocomplete (route-based)"),
    + *   field_types = {
    + *     "entity_reference",
    + *   }
    + * )
    ...
    +      '#autocomplete_route_name' => $this->getSetting('route_name'),
    

    As the route_name setting has to be set from code, I think this shouldn't be available from the UI, right? I do not see a no_ui here, though. (And I think that's not even possible for widgets ATM.)

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/TimestampWidget.php
    @@ -0,0 +1,81 @@
    +    $element['value']['#description'] = $this->t('Format: %time. The date format is YYYY-MM-DD and %timezone is the time zone offset from UTC. Leave blank to use the time of form submission.', array('%time' => !empty($default_value) ?
    +        date_format(date_create($default_value), 'Y-m-d H:i:s O') : format_date($created_timestamp, 'custom', 'Y-m-d H:i:s O'), '%timezone' => !empty($default_value) ? date_format(date_create($default_value), 'O') : format_date($created_timestamp, 'custom', 'O')));
    

    I cannot even remotely parse what this line is doing. Please split it into multiple lines.

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/TimestampWidget.php
    @@ -0,0 +1,81 @@
    +      if ($date->hasErrors()) {
    +        $value = FALSE;
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeTimestampWidget.php
    @@ -0,0 +1,62 @@
    +    if ($date->hasErrors()) {
    +      $value = -1;
    

    I do not understand the discrepancy here.

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/TimestampWidget.php
    @@ -0,0 +1,81 @@
    +    form_set_value($element, $value, $form_state);
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeTimestampWidget.php
    @@ -0,0 +1,62 @@
    +    form_set_value($element, $value, $form_state);
    

    You used \Drupal::formBuilder() above, so can we do it here, as well? Again, protected function formBuilder() would be preferred.

  9. +++ b/core/modules/datetime/datetime.module
    @@ -978,21 +978,13 @@ function datetime_range_years($string, $date = NULL) {
    -function datetime_form_node_form_alter(&$form, &$form_state, $form_id) {
    ...
    -function datetime_node_prepare_form(NodeInterface $node, $operation, array &$form_state) {
    

    YAY!!! :-D

  10. +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeTimestampWidget.php
    @@ -0,0 +1,62 @@
    +    $date_format = entity_load('date_format', 'html_date')->getPattern();
    +    $time_format = entity_load('date_format', 'html_time')->getPattern();
    

    Similar to above please use a protected function entityManager() or alternatively dateFormatStorage().

    Oh wait, we have DateFormat::load() now. So while the former would still be preferred the latter would at least be an improvement.

  11. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceIntegrationTest.php
    @@ -75,9 +76,14 @@ public function testSupportedEntityTypesAndWidgets() {
    +      entity_create('user', array(
    
    @@ -100,9 +106,14 @@ public function testSupportedEntityTypesAndWidgets() {
    +      entity_create('user', array(
    

    These are tests, so you don't have to go all injection-y here, but at least User::create() would be nice.

  12. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceIntegrationTest.php
    @@ -166,9 +177,9 @@ protected function getTestEntities() {
    +    $content_entity_1 = entity_create('entity_test', array('name' => $this->randomName(), 'user_id' => 0));
    ...
    +    $content_entity_2 = entity_create('entity_test', array('name' => $this->randomName(), 'user_id' => 0));
    

    Again, EntityTest::create() would be preferred.

  13. +++ b/core/modules/node/node.module
    @@ -204,20 +212,6 @@ function node_entity_view_display_alter(EntityViewDisplayInterface $display, $co
    -function node_entity_form_display_alter(EntityFormDisplayInterface $form_display, $context) {
    

    Yay!

  14. +++ b/core/modules/node/node.module
    @@ -635,14 +629,10 @@ function template_preprocess_node(&$variables) {
    +  $variables['date'] = drupal_render($variables['elements']['created'], TRUE);
    

    Is the TRUE required or just a performance optimization. Although it sort of makes sense, it seems like that is something that Contrib could get wrong easily. Or would contrib not have to do anything similar?

  15. +++ b/core/modules/node/src/Entity/Node.php
    @@ -386,10 +386,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    -      ->setSetting('target_type', 'user')
    ...
    +      ->setSettings(array(
    +        'target_type' => 'user',
    +        'default_value' => 0,
    +      ))
    

    Until #2236903: setSettings() on field definitions can unintentionally clear default values gets in using setSetting() multiple times instead actually is mandatory over setSettings().

  16. +++ b/core/modules/node/src/Entity/Node.php
    --- a/core/modules/node/src/NodeForm.php
    +++ b/core/modules/node/src/NodeForm.php
    

    Oh, I just love all the removals in this class. Yay!

  17. +++ b/core/modules/node/src/NodeTranslationHandler.php
    @@ -80,8 +80,9 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
    +      $account = user_load($form_state['values']['uid'][0]['target_id']);
    

    At least User::load() would be preferred. AFAIK translation handlers actually have a createInstance() so we could properly inject the entity manager or even the user storage here.

  18. +++ b/core/modules/node/src/Tests/PageEditTest.php
    @@ -114,21 +114,21 @@ function testPageAuthoredBy() {
    +    $this->assertRaw(format_string('The username %name does not exist.', array('%name' => 'invalid-name')));
    

    Please use Format::string().

  19. +++ b/core/modules/rdf/rdf.module
    @@ -233,7 +233,7 @@ function rdf_comment_load($comments) {
    +    $comment->rdf_data['date'] = rdf_rdfa_attributes($created_mapping, $comment->get('created')[0]->getValue());
    
    @@ -305,7 +305,7 @@ function rdf_preprocess_node(&$variables) {
    +    $date_attributes = rdf_rdfa_attributes($created_mapping, $variables['node']->get('created')[0]->getValue());
    

    The [0] should be handled automatically, I think. Otherwise, please use first().

  20. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWidget.php
    @@ -18,7 +18,8 @@
    + *     "text_long",
    + *     "string_long",
    

    I think this is fine for now, but this is problematic as string_long does not support text formats. Not sure what would happen with a string_long field type if I set text_processing to TRUE. Can we at least add a @todo here to add a dedicated formatter?

tstoeckler’s picture

So I would ask that 1,3,5,6,7,14,15,19,20 be adressed in some form, if possible. The rest would be nice as well, but since this has been so much work already please feel free to ignore and I will open a follow-up. Thanks!

Berdir’s picture

Status: Needs work » Needs review
FileSize
59.62 KB

Just a re-roll for now.

Berdir’s picture

Status: Needs work » Needs review
FileSize
58.97 KB
8.46 KB

Ups, missed a few merge conflicts.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Avoid commit conflicts
FileSize
59.73 KB
2.35 KB

Fixing those test fails.

alansaviolobo’s picture

Berdir’s picture

1. Moved.
3. Moved. Did not change the other things yet.
4. This is a widget, so it should directly use the field item, not go through the entity, not sure why it did that, we'll find out if there are test fails I guess. Not sure about the name of those two classes, maybe it should be User instead of Author, as it's not limited to be used for the Author/Owner?
5. It being a setting has nothing to do with UI, there would only be a UI if we'd implement a settings form, which we don't. So it's a setting that has to be set in code.
6. Rewritten that, also used REQUEST_TIME as an example, no need to depend on getCreatedTime(), especially as it's just a example value *and* it would actually be the same as the $default_value in this case.
7. Neither do I, but i guess that's existing code, not sure about touching that here. They're using completely different form types, so I'd expect that there are differences.
12. EntityTest::create() actually doesn't work because of the changes that @fago made me do :( My mistake, but I blame him :p (We broke the real use case of having (multiple) subclasses by trying to implement the theoretical use case of the same class being used for multiple entity types).
14. Pretty sure this is a performance optimization only, to avoid further cache collects. @Wim: Can we use that as a quick fix/workaround for the insanely slow testing table rendering, and make sure that this is passed to the sub-theme functions for theme_table() ? I see that table is now a twig template, not sure how that affects things exactly.
17. translation handlers are nothing but a bunch of hacks put in a class, that's not going to make it prettier ;)
18. I think you meant String::format() :p
19. It's only handled automatically if you access a single property, getValue() gives you a list of items with their values otherwise. Still consider that code crazy ugly, we shouldn't rely on that structure, but I made it a ->first()->toArray() for now at least.
20. Not sure that is necessary, I don't think anything would actually break and it's not an existing, setting of that field type, so you're not allowed to set it anyway. And if we really wanted to, we could check if the item has a format property additionally to the setting.

Open:
2., parts of 3., 8, 10, 11, 12, 17, 20.

Berdir’s picture

Status: Needs work » Needs review
FileSize
62.09 KB
3.25 KB

Interesting test fail ;)

I think we need to introduce no_ui for widgets too, because RouteBasedAutocompleteWidget (should this even be a widget, it doesn't seem to be usable without a value massing/validation method?) really shouldn't show up in the UI, and author also not, or we'd need a supports($field) method on the widget, because it only works for user references.

yched’s picture

Yes, "author widget" intrigued me on a very cursory read. What's the need for a dedicated, specific widget ?

The problem with making those "internal widgets only" is that the base fields that use them cannot have their "form display settings" exposed in the UI either (since their initial widget is not available in the UI).

Letting people switch-in powerful entity ref widgets from contrib (like the very promising https://www.drupal.org/project/entity_browser) was one of the big wins about moving "author" to a field + widget...

Berdir’s picture

The entity reference widgets are provided by the module, using it from node would require to make entityreference practically a required module. We can't just move it to Drupal\Core as it depends on the whole selection plugin stuff.

I think @Wim tried to avoid making any functional changes, and this would be one example that would be one, the empty behavior is different (defaults to anonymous user if you leave it empty, the entity reference widgets would have to be required and explicitly selected), the autocomplete selection behavior would be different (it mostly relies on the ID, when given, the current one just matches the username). And at least when this issue started, the exact-single-match mode without ID in entityreference was broken (and still is in 7.x :(, for a long time now...).

Also, it's quite likely that the entity reference widgets only worked for configurable fields when we started this issue, we fixed that not too long ago.

All that said, I agree that we should seriously consider to just rely on entityreference, use that widget and move on, as we'll get a lot of nice stuff for free. I'll try to switch, then we'll see what happens with the tests.

Berdir’s picture

Let's see what happens. At least the selecting the anonymous user seems to be broken, autocomplete displays it but then validation fails.

Berdir’s picture

Status: Needs work » Needs review
FileSize
64.73 KB
4.25 KB

Ok, fixed those fails (could not reproduce quickedit after fixing the others), we do have a UX regression on the validation message :(

Also, did not yet switch the formatter.

Berdir’s picture

now also using the entity reference formatter.

Berdir’s picture

Status: Needs work » Needs review

The test fails are because of a bug in EntityReferenceFormatterBase::prepareView() but anyway, AuthorFormatter is something we need to keep I guess, except we add a rel option to the EntityReferenceLabel formatter...

So, back to #88, for decision about depending on entity_reference or not. We did introduce the date alters to avoid a dependency on datetime in node.module, but that was before we removed it as a testing install profile dependency.

andypost’s picture

+++ b/core/modules/node/src/Entity/Node.php
@@ -421,9 +462,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
     $fields['revision_log'] = FieldDefinition::create('string_long')
...
+      ->setDisplayOptions('form', array(
+        'type' => 'text_textarea',

could use "string_long" after #1856562: Convert "Subject" and "Message" into Message base fields

Wim Leers’s picture

#81.14: It's not a performance optimization; it's to ensure that cache tags are bubbled up.

#84: "author widget": IIRC this was necessary because of the special case that we have for User entity references: if blank, use the anonymous user. And I think that on top of that there's also <name> being allowed instead of only <name> (<ID>). In other words: special snowflake behaviors on top of the basic behavior, for a better UX.

#85: indeed.

andypost’s picture

FileSize
1007 bytes
9.11 KB

a bit of clean-up

andypost’s picture

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
67.36 KB
Wim Leers’s picture

This patch was RTBC'd 1.5 months ago in #60. Then it was broken twice by commits. Then tstoeckler came in and asked for things that didn't exist until very shortly before the patch was RTBC (<entity type>::create() etc.) — which is fine, but not very helpful for a patch that easily conflicts with other commits.

Then I was working on a big beta blocker ("menu links home stretch"), which didn't leave me any time to reroll this one. Berdir then rerolled this and addressed tstoeckler's feedback (thanks! :)).

And now it seems this issue is blocked on deciding whether it should rely on the standard entity reference widgets. I'd love to see that happen also. But, that is nontrivial due to the special behavior for the author autocomplete that we've had so far (see #85 and #94). On top of that, I think it's a nice-to-have. Please let us get this in, it converts all node base fields to use formatters and widgets. Once we have that, it's much easier to move from there to the standard entity reference widget.

Please let us get this in ASAP. We've been rerolling this for almost 4 months. It took us >2 months to get it to green. Since May 31, it's only been broken by commit conflicts and it's only been criticized for not having things that have only become possible after May 31. Let's not let Perfect be the enemy of Better. Let's get this in, and then make further incremental improvements.

Berdir’s picture

And now it seems this issue is blocked on deciding whether it should rely on the standard entity reference widgets. I'd love to see that happen also. But, that is nontrivial due to the special behavior for the author autocomplete that we've had so far (see #85 and #94). On top of that, I think it's a nice-to-have. Please let us get this in, it converts all node base fields to use formatters and widgets. Once we have that, it's much easier to move from there to the standard entity reference widget.

See the discussions above. adding the custom widgets there results in them showing up the UI, which makes no sense as they need configuration that can't be entered in the UI to function. So we would need to introduce a system to hide widgets like we have for field types.

The same is true for the author formatter. Not sure what to do there.

Also, I don't really see the different anonymous behavior as a problem, there was exactly one test fail due to that. And as discussed, entity reference supports just "Username" too, or we would have had more fails.

Wim Leers’s picture

#2226063: Merge ListBooleanItem from options module into BooleanItem landed; now we need to reroll this, and it'll become a bit simpler yet again :) Berdir++

YesCT’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
63.14 KB
1.46 KB

Re-roll.

Most of those fails were due to the missing + on the revision_log addition I think.

Also removed the new Boolean widget, using the existing one now. Removal is not visible in the interdiff, the resulting change for the display options is.

Berdir’s picture

Removed the no longer used widgets, added schema for datetime_timestamp, fixed other missing "+"'s and updated tests for the new checkbox widget.

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/TimestampWidget.php
    @@ -0,0 +1,86 @@
    +   * @todo Convert to massageFormValues() after https://drupal.org/node/2226723 lands.
    

    That issue has been closed as "cannot reproduce". So we should try to find out if there really is an issue ?
    (same in DateTimeTimestampWidget)

  2. +++ b/core/modules/node/src/NodeTranslationHandler.php
    @@ -80,8 +80,9 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
    -      $translation['created'] = $form_state['values']['created'];
    ...
    +      $translation['created'] = format_date($form_state['values']['created'][0]['value'], 'custom', 'Y-m-d H:i:s O');
    

    Will this still work when the datetime.module is enabled and changes the 'created' widget to DateTimeTimestampWidget ?

  3. +++ b/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php
    @@ -0,0 +1,50 @@
    +class AuthorFormatter extends FormatterBase {
    

    So this is the last problematic formatter - it is eligible and will show up in the "Manage display" UI for all entity_ref fields, but only makes sense for entity_ref fields on users (also, the current code will probably have weird effects / break when used on a non user entity_ref).

    Since the 'uid' base field is not set to have its display configurable in the UI, maybe we don't need a formatter for this, and could stick with hardcoding the theme_username() in template_preprocess_node() from the raw field value ?

    That would remove the need for the custom field--node--uid.html.twig.

    (maybe we should do something similar for 'created': render the value with a formatter in template_preprocess_node(), and lose the speciic field template ?)

    (Also, that 'uid' field should be renamed 'author', really - but that's probably be too big for this issue here, opened #2313799: Rename node 'uid' field to 'author')

Berdir’s picture

Re not using formatters.. I think using formatters is Wim's main motivation for these issues, as it allows editor/quickedit to work with those fields, that is not possble if we keep the manual output.

3. Specifically for this one, I was hoping that we could use #2204325: The "rendered entity" formatter breaks for entity types with out a ViewBuilder to only display that option for references to user entities. For those, having a formatter that displays the reference with a rel="author" seems like a useful thing? Could use your opinion on that issue.

yched’s picture

Formatters / quickedit: ah, sure, silly me.

Will look at that other issue.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
57.65 KB

Rerolled. The problem occurred because of introduced FormStateInterface in datetime.module file.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
57.25 KB
3.9 KB

rererolled

m1r1k’s picture

Status: Needs work » Needs review
Issue tags: +Drupalaton 2014
FileSize
57.67 KB
1.25 KB
m1r1k’s picture

Status: Needs work » Needs review
FileSize
57.73 KB
1.11 KB

Definitely the last fix of FormStateInterface.

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/TimestampWidget.php
    @@ -0,0 +1,87 @@
    +  public function elementValidate($element, &$form_state, $form) {
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeTimestampWidget.php
    @@ -0,0 +1,63 @@
    +  public function elementValidate($element, &$form_state, $form) {
    

    2 more left

  2. +++ b/core/modules/node/src/NodeTranslationHandler.php
    @@ -81,8 +81,9 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
           $translation['status'] = $form_controller->getEntity()->isPublished();
    ...
    -      $translation['created'] = $form_state->getValue('created');
    ...
    +      $translation['created'] = format_date($form_state['values']['created'][0]['value'], 'custom', 'Y-m-d H:i:s O');
    

    created could be accessed via populated entity, is not it?

m1r1k’s picture

Status: Needs work » Needs review
FileSize
5.16 KB
59.13 KB

1), 2) Fixed.

Also i've removed custom validation of created and author fields from NodeForm as they are fields now and have own validation.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
60.96 KB

Add default value callback for uid basefield.
Fixed node.js script.
Fixed NodeForm->buildEntity method.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
1.83 KB
61.32 KB

Another attempt :)

andypost’s picture

Status: Needs work » Needs review
FileSize
6.15 KB
61.92 KB

A bit fixed interdiff from @m1r1k

Revision log now has field access control after #2098355: Missing default access for all node fields

m1r1k’s picture

m1r1k’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
62.5 KB

Previous patch contained one useless modification and interdiff was invalid. Here is an update.

yched’s picture

Status: Needs review » Needs work

Now that #2204325: The "rendered entity" formatter breaks for entity types with out a ViewBuilder was committed, AuthorFormatter should use the added isApplicable() method to only show on entity ref fields targetting users.

cbr’s picture

Assigned: Unassigned » cbr
FileSize
62.36 KB

rerolled, will be working on #134

cbr’s picture

Status: Needs work » Needs review
Wim Leers’s picture

@m1r1k et al: As you can tell, it's quite noisy to upload patches just to see what testbot's verdict will be. For complex patches (like this one), where the likelihood of test failures is high, we generally prefer to post patches to "helper issues" (like #2268971: Helper for #1805054), and then once we have a green patch, we post it to the actual issue.

Status: Needs review » Needs work

The last submitted patch, 135: 2226493-node-base-135.patch, failed testing.

cbr’s picture

Status: Needs work » Needs review
FileSize
62.63 KB
3.29 KB

Added isApplicable() method to only show on entity ref fields targeting users and provided test coverage for this in EntityReferenceAdminTest.php.

andypost’s picture

  1. +++ b/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php
    @@ -47,4 +48,7 @@ public function viewElements(FieldItemListInterface $items) {
    +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    +    return $field_definition->getFieldStorageDefinition()->getSetting('target_type') == 'user';
    

    Should be just:
    return $field_definition->getSetting('target_type') == 'user';

  2. +++ b/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php
    @@ -47,4 +48,7 @@ public function viewElements(FieldItemListInterface $items) {
    +  }
     }
    

    Leave an empty line between end of method and end of class definition
    https://www.drupal.org/node/608152

Berdir’s picture

1. No, the target_type is a field storage setting, that is correct.

Berdir’s picture

+++ b/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php
@@ -47,4 +48,7 @@ public function viewElements(FieldItemListInterface $items) {
 
+  public static function isApplicable(FieldDefinitionInterface $field_definition) {
+    return $field_definition->getFieldStorageDefinition()->getSetting('target_type') == 'user';
+  }

Missing the @inheritdocs docblock.

andypost’s picture

Assigned: cbr » yched
FileSize
2.61 KB
62.8 KB

Fixed a few doc-blocks, suppose we need @yched to fire rtbc

yched’s picture

I don't think points 1 and 2 from #109 have been answered yet ?

Wim Leers’s picture

Besides #144, there's one more thing: since #2098355: Missing default access for all node fields landed, we should be able to remove the #access stuff in NodeForm; which means almost everything of the node form will be generated!

yched’s picture

Status: Needs review » Needs work

#145 means NW ?

Berdir’s picture

Already working on this and the @todo's.

yched’s picture

Also, I'm thinking we should probably get #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI in first, it clarifies the roles for string* anf text* field types and the corresponding widgets (which will likely clash with what is made in this patch)

Berdir’s picture

Status: Needs work » Needs review
FileSize
63.19 KB
6.08 KB

This seems to work, the reason the timestamp massaging didn't work I think was there was a left-over code in buildEntity() that didn't work anymore as expected because created was always there but as "array('value' => '')", so empty said FALSE.

That said, validation is a problem now. With the core timestamp widget, we get that insanely stupid "This value should be of the correct primitive type.", and with datetime, weird stuff happens, we have no place where validation is done anymore, so it tries to save NULL if I pass around NULL, and in the current code -1. It is also, already in HEAD, easily possible to crash it by either selecting 2050 or 1900 in the widget because the value is then too large. Sounds like we need a max/min validation on the timestamp, but that again will result in a meaningless validation error. :(

Berdir’s picture

#148: The only place where that is relevant, I think is the revision_log widget, and I just changed that to string_textarea now, forgot to do that before. Now the other issue shouldn't affect this anymore?

The last submitted patch, 149: 2226493-149.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 150: 2226493-150.patch, failed testing.

yched’s picture

Re #150: yay! Then we should also remove the hunk in TextareaWidget that adds string_long as an applicable field type - it has no real sense, and is the part that would clash with #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI (at least conceptually, if not patch-wise)

Berdir’s picture

Status: Needs work » Needs review
FileSize
62.89 KB
1.88 KB

Hm, massageFormValues() somehow gets inconsistent values...

Status: Needs review » Needs work

The last submitted patch, 154: 2226493-154.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
62.89 KB

Re-roll.

yched’s picture

I'm still reviewing, but reacting to the last couple comments:

  1. +      // @todo Why is the structure different when the field is visible or not?
    

    You mean when #access is TRUE or FALSE ? Ouch, yeah, there might be an issue with here.
    That would require investigation in its own issue. I opened #2326533: The structure of values received in WidgetInterface::massageFormValues() varies depending on #access TRUE/FALSE (@Berdir, please confirm/adjust the issue summary there).

    Meanwhile, we can keep the workaround you put in place with a @todo on the above ?

  2. From interdiff #149:
    +++ b/core/modules/node/src/NodeForm.php
    @@ -129,7 +129,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -      '#access' => $node->isNewRevision() || $current_user->hasPermission('administer nodes'),
    +      '#access' => $node->isNewRevision() || $form['revision_log']['#access'],
    

    Oh ? That feels a bit backwards, deriving the #access of the group from the #access of another (arbitrary ?) field rather than on proper permissions ?

  3. Validation issues raised in #149:
    Not sure I completely follow, and so not clear on the possible way forward.

    - Is that a consequence of moving to massageFormValues() ?

    - "This value should be of the correct primitive type": don't we have an issue to allow constraints to pass more telling fail messages ? If so, isn't just a @todo on that known issue ?

    - "with datetime, weird stuff happens, we have no place where validation is done anymore"
    Sounds bad - where was this validation done so far ?

    - "Sounds like we need a max/min validation on the timestamp, but that again will result in a meaningless validation error. :("
    Would that then be the same "@todo [custom validation message issue]" than above ?

  4. 'created' was always there but as "array('value' => '')", so empty said FALSE

    "false non-empties" are a real pain :-/. We bump into them less often because we have sprinkled random ->filterEmptyItems() calls all over the place, but they still appear very easily (just ->get(non existent delta) and it creates one) and so we're still very brittle on that.
    I still think we should fix #2164601: Stop auto-creating FieldItems on mere reading of $entity->field[N], but I got a bit stalled there... (also, the code was a bit of a shoot in the dark)

Berdir’s picture

1. Yes, I think that is exactly the problem I saw. Will clarify the @todo and reference that issue.

2. Yes, I'm not sure about that. The revision_log field is visible when you have permission to toggle new revisions *or* when you don't but the new revision checkbox is already ticked. So it's more a UI thing than something else. Not sure that is something that belongs in field access? Not showing it is more a UI thing than anything else, it's not that you wouldn't be allowed to write in it, it just won't be used and would be misleading.

3. Date fields on the node form are a huge mess, there are existing issues, see #2031183: Improve test coverage for node authored on widget and #2315023: Saving a node without having the datetime module enabled updates the created field always ( think they are duplicates, more or less). And this makes it worse, part of the validation that we had is removed in favor of non-existing field validation ( no max/min for what we can actually store, only the weird integer scalar check) because there's quite the mismatch between the field type (timestamp) and UI which shows date strings, which we can only convert to a timestamp if it's valid in the first place, we can't validate a timestamp to see if it's a valid, so that validation has to happen in the UI/widget.

At some point, node.module depended on datetime.module for that widget, we changed that to be optional as node was then still part of testing profile, and we wanted to avoid enabling datetime in the testing profile. This has been a mess since at least then, probably longer. That changed, so I'm considering to drop that and just rely on datetime.module again.

4. I know that issue, but I don't think it would help here. There actually *is* a field item (after all, we just displayed a widget for it), but the user left it explicitly empty. As mentioned, I think the what Wim saw as massageFormValues() being broken was really the combination of massageFormValues() and that removed snippet there, which was the last line of defence against an empty created date that then no longer worked.

Berdir’s picture

FileSize
63.12 KB
970 bytes

Reroll and updated the @todo.

yched’s picture

- Regarding 2. and

$form['revision_log'] += array(
  '#access' => $node->isNewRevision() || $form['revision_log']['#access'],
);

So $form['revision_log']['#access'] comes in populated with what gets assigned by the field permissions, right ?
So what this does is discard field permissions to always show the input field if isNewRevision() ?

Yeah, not sure if that "isNew()" behavior should be baked in the 'revision_log' field permissions in NodeAccessControlHandler::checkFieldAccess(), or be adjusted directly in the form as "UI thing only".
Then again, the current code means we write a field value for which the permission says "no write grants" ? Isn't that weird ? Don't we want to enforce that same behavior on REST writes ?

At any rate, if we do leave it in $form, this snippet might deserve a comment, as that's not too straightforward on first read (I misread it at first)

- Regarding 3. and date / validation messages
Yeah, switching the widget depending on whether datetime.module is enabled feels really awkward. Not sure whether the datetime widget can/should be moved to Core, or if node should depend on datetime.module, but the current situation feels like a WTF.

Then again, even if node depends on datetime, doesn't it just leave the issue for other entities that also want to store creation dates ?

All in all, not too sure of the way forward there...

yched’s picture

Actually, yes, not sure why DateTimeTimestampWidget can't move to Core ?

Berdir’s picture

Here are the three possible states for revision_log:

- administer nodes permission: #access TRUE, visibility with #states based on revision checkbox.
- new revision enabled by default, no administer permission: revision_log always visible
- new revision disabled by default, no administer permission: revision log always hidden

We can probably implement that logic in field access, if we want to do that. not 100% sure, but I can try. It is not possible to create new revisions over rest anyway, that's kind of the bigger problem ;) (or do anything with revisions)

3. datetime.module currently provides a lot of elements and theme functions. both for form elements/editing and displaying. While element stuff can now actually live in Core, theme functions is afaik still a bit awkward (=system.module AFAIK). comment, unlike node, depends on datetime.module.

yched’s picture

- administer nodes permission: #access TRUE, visibility with #states based on revision checkbox.
- new revision enabled by default, no administer permission: revision_log always visible
- new revision disabled by default, no administer permission: revision log always hidden

Then would it make sense to have:
- field permission = ('administer nodes' perm) || (new revision enabled by default for the node type)
- UI #states sugar in the $form to only show the textarea when it actually makes sense (if the revision checkbox is (checked) || (hidden because forced to "checked")
?

datetime.module: sure, moving it to Core sounds like a valid but non-minor effort, but I was talking specifically about the DateTimeTimestampWidget - I see nothing in there that couldn't be moved to Core as is ?

Berdir’s picture

It uses #type datetime, which is defined in datetime.module. It would be much easier to move that now that hook_element_info() are plugins, but would still involve quite some callbacks and stuff I guess.

The #states stuff already exists, so that access might work yes, I'll try it out.

yched’s picture

#type datetime - doh, right, silly yched.
Oh gee. Yes, moving that #type to an Element class would sure help make things clearer, but that's for another issue.

As a side note, opened #2327363: Move datetime_datetime_widget_validate() to DateTimeDefaultWidget::massageFormValues() (but that's not really related)

Berdir’s picture

Trying the revision_log access changes, with tests.

yched’s picture

Berdir’s picture

Nice, Tim being awesome again :)

Moved the datetime widget to Core\Datetime (not 100% sure if it should be there or in field, but I think it makes sense there, but can't be the default then), removed the other one, removed the alter stuff.

Improved validation by limiting the years to a safe value (can't make it exact dates, so playing safe), and also added a range validator to the timestamp item.

jibran’s picture

Issue tags: +Twig
+++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidget.php
@@ -73,7 +73,7 @@ public function elementValidate($element, FormStateInterface $form_state, $form)
-      if (!$value) {
+      if ($value === NULL) {

What is the reason for this change?

Status: Needs review » Needs work

The last submitted patch, 168: 2226493-168.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
62.85 KB
12.75 KB

The reason for that change is that "Anonymous (0)" results in $value = 0, which is a valid result, so we need to do a type safe check.

Went through the patch again, cleaning up things. A bit confused by NodePreviewTest, I thought that I've fixed that properly before, the right fix is not to give the user admin permission but to configure the node type properly, instead of the bogus code that was there before.

Similar for EntityTranslationFormTest, I just reverted that to the original test and removed the bogus :? FALSE (because it's never used anyway with the old code and it's all working just fine. I suspect the changes there date back to attempts to create a widget for langcode.

yched’s picture

Still have a couple files to review, posting what I have for now (minor stuff)

  1. +++ b/core/modules/entity/src/Tests/EntityDisplayTest.php
    @@ -285,19 +285,23 @@ public function testRenameDeleteBundle() {
    +    $expected_view_dependencies = array(
    +      'entity' => array('field.instance.node.article_rename.body', 'node.type.article_rename'),
    +      'module' => array('text', 'user')
    +    );
    +    $expected_form_dependencies = array(
    

    minor: each var could be declared above the hunk that actually tests it ?

  2. +++ b/core/modules/node/src/Tests/PageEditTest.php
    @@ -108,21 +108,21 @@ function testPageAuthoredBy() {
         // Change the authored by field to an empty string, which should assign
         // authorship to the anonymous user (uid 0).
    -    $edit['uid'] = '';
    +    $edit['uid[0][target_id]'] = 'Anonymous (0)';
    

    Comments needs an update

  3. +++ b/core/modules/rdf/rdf.module
    @@ -305,7 +305,7 @@ function rdf_preprocess_node(&$variables) {
    +    $date_attributes = rdf_rdfa_attributes($created_mapping, $variables['node']->get('created')->first()->getValue());
    

    The other replacements in that file use ->toArray() rather than ->getValue() ?

  4. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationFormTest.php
    @@ -58,24 +58,24 @@ function testEntityFormLanguage() {
         $edit = array();
         $edit['title[0][value]'] = $this->randomMachineName(8);
         $edit['body[0][value]'] = $this->randomMachineName(16);
    

    Apparently the var is not used anymore by the code below it, should be removed ?

Berdir’s picture

1. Yes, moved it.
2. Updated.
3. Switched to toArray().
4. I assume you reviewed the second last patch? I reverted almost all changes in that test in the last patch, so it is no used again.

Also cleaned up NodeController because the uid is now set by default. This means that this essentially solves #1979260: Automatically populate the author default value with the current user too. It's not very nice, but it works.

yched’s picture

OK, finished reviewing at last :-)

  1. +++ b/core/lib/Drupal/Core/Datetime/Plugin/Field/FieldWidget/TimestampDatetimeWidget.php
    @@ -0,0 +1,68 @@
    +namespace Drupal\Core\Datetime\Plugin\Field\FieldWidget;
    ...
    +class TimestampDatetimeWidget extends WidgetBase {
    

    Core/Datetime does provides the base FAPI #types, but Core/Field provides the TimestampItem field type and the TimestampFormatter - would it be really problematic if TimestampDatetimeWidget was in Core/Field too ?

    Would mean Core/Field contains dependencies on Core/Datetime, but the patch currently does the opposite, so it would not really be worse ?

    Would be more natural IMO, and let TimestampItem / CreatedItem use it as their 'default_widget' ?

  2. +++ b/core/modules/entity/config/schema/entity.schema.yml
    @@ -137,3 +137,13 @@ entity_form_display.field.string:
    +entity_form_display.field.datetime_timestamp:
    

    The datetime_timestamp widget is in Core, so we shouldn't be adding its config schema in entity.module ?

  3. +++ b/core/modules/node/node.module
    @@ -608,14 +607,10 @@ function template_preprocess_node(&$variables) {
    -  $variables['date'] = format_date($node->getCreatedTime());
    -  $username = array(
    -    '#theme' => 'username',
    -    '#account' => $node->getOwner(),
    -    '#link_options' => array('attributes' => array('rel' => 'author')),
    -  );
    -  $variables['author_name'] = drupal_render($username);
    +  $variables['date'] = drupal_render($variables['elements']['created'], TRUE);
    +  unset($variables['elements']['created']);
    +  $variables['author_name'] = drupal_render($variables['elements']['uid'], TRUE);
    +  unset($variables['elements']['uid']);
    

    does this need to be re-evaluated in light of #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render ? @Wim ?

  4. +++ b/core/modules/node/src/Entity/Node.php
    @@ -372,10 +372,27 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
         $fields['uid'] = BaseFieldDefinition::create('entity_reference')
    ...
    +      ->setDisplayOptions('form', array(
    +        'type' => 'entity_reference_autocomplete',
    

    Why don't we add setDisplayConfigurable('form', TRUE) ?
    We want to allow rich contrib entity_ref widgets there, don't we ? (can be a followup if we suspect dragons...)

  5. +++ b/core/modules/node/src/NodeForm.php
    @@ -129,13 +126,17 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form = parent::form($form, $form_state, $node);
    

    Moving the parent:: call at an arbitrary position in the middle of the method is a bit surprising, might be worth a comment explaining why it's done there ?

    (also, can't it move at the beginning instead ? I can see the "Create the "advanced" vertical tabs before building the form, so that field widgets may detect its presence and choose to live there." part, but I'm not sure how widgets could actually do that ?)

  6. +++ b/core/modules/node/src/NodeForm.php
    @@ -197,26 +195,15 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form['uid']['widget'][0]['value']['#title'] = $this->t('Authored by');
    ...
    +    $form['created']['widget'][0]['value']['#title'] = $this->t('Authored on');
    

    Why don't we just set those as the field labels in the base feild definitions ?

  7. +++ b/core/themes/bartik/templates/node.html.twig
    @@ -86,7 +84,7 @@
    -          {% trans %}Submitted by {{ author_name|passthrough }} on {{ date }}{% endtrans %}
    +          {% trans %}Submitted by {{ author_name|passthrough }} on {{ date|passthrough }}{% endtrans %}
    

    This is only done in bartik, not in node.module/templates/node.html.twig ?

Berdir’s picture

1. Yeah, I'm not sure there, would be great to get some other opinions on that. Core\Datetime also has an entity type now, so it depends on Core\Entity already. I don't really care, I'm can live with both options.

2. We can move it to core when the entity.module no longer required issue is committed, which will also move the others. However ,right now, it extends from a type that is in entity.schema.yml, so that seems like the right place ATM. Hopefully the right place for a very short time :)

3. Leaving that to Wim.

4. One reason there are groups. While we can make it configurable and switch the widget, we can't movei t outside of those groups, as the manage display screens have no understanding of that.

5. I had to move it a bit after the preview patch, not sure why exactly it's not first. I can try to move it first, we'll see what happens then.

6. Was wondering that too, fine with me, I don't think t('Created') is a good label :)

yched’s picture

One reason there are groups. While we can make it configurable and switch the widget, we can't movei t outside of those groups, as the manage display screens have no understanding of that

Yes, but isn't that also true for other fields where we allow teh widget to be configurable ?

No reply on 7 ?

moshe weitzman’s picture

3. It used to be a really bad idea to render stuff in a preprocess since the cache tags would get dropped on the floor. Now, it is still discouraged but not as big of a problem as the cache tags are passed up the stack. So, this issue doesn't need to tackle the bad form. Proceed!

yched’s picture

@moshe / @Wim: Is the second param on drupal_render() still needed though ?
(just wondering)

Berdir’s picture

Fixed the parent call, labels and node.html.twig.

Berdir’s picture

@yched: Yeah, created is set to TRUE. I don't know why some are and others not, maybe uid wasn't because it was a custom widget?

Status: Needs review » Needs work

The last submitted patch, 179: 2226493-176.patch, failed testing.

effulgentsia’s picture

@moshe / @Wim: Is the second param on drupal_render() still needed though ?

For now, yes. If you invoke drupal_render() from within rendering (i.e., in #pre_render, preprocess, or theme function), you need to pass in TRUE. If you don't, then if the element has a #post_render_cache on it, it'll get run twice. There might be places not passing it in and still working, either because they happen to not use #post_render_cache or because the #post_render_cache function happens to be idempotent, but where possible, we shouldn't rely on those accidents. Perhaps some day, we'll find a way to remove that parameter.

effulgentsia’s picture

If you don't, then if the element has a #post_render_cache on it, it'll get run twice.

Actually, it's worse than that. When you call drupal_render() without setting the 2nd param to TRUE, its #post_render_cache functions will run, and then a parent element might have #cache set, and we'll end up render caching the expanded HTML, which would result in a cache poisoning bug.

yched’s picture

@effulgentsia: thanks for the explanation. That second param (or rather knowing when to set it or not, the effects of not setting it when you should and reversedly) sounds a bit worryingly brittle, but well, out of scope here.

@Berdir : yeah, IIRC at some earlier point the patch used a dedicated AuthorWidget for node.uid because the regular entity_ref widgets wouldn't work. I think this was fixed meanwhile, right ? So we could relax the constraint on the widget now ?

@Wim, does that ring a bell ?

Anyway, as I said, this can also be an easy followup. This is RTBC for me when it's green again. Thanks a lot @Berdir & @Wim !

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.83 KB
66.27 KB

It's really hard to stop here. Had a look at the ui configurable thing and noticed an interesting problem with the current code: If you hide such a configurable widget, the code blows up because we did an unconditionaly += on the form array. Replaced that with isset checks now and made uid, stick and promote also UI configurable.

The reason I added it for those too is that I basically sold this issue as an alternative/additional fix for #987242: The "Promoted to front page" checkbox doesn't do anything if the /node front page listing isn't used: if you don't need promote/sticky for some node types, just hide it in the form display.

Also reverted the parent::form() call change, there's some preview $form_state replacement going on above those lines that must happen first.

And while doing that, I also noticed the bogus anonymous user name setting and logic for it in node.js, removed that too. And when testing that, I noticed that the sticky/promoted summary includes the description, not sure if caused by the change to a widget or not. There's more cruft in there, like translate options that were removed years ago. (We don't show this stuff anymore in seven, but it is still there...)

But as I said.. must. stop. making. more. changes..

The last submitted patch, 185: 2226493-185.patch, failed testing.

effulgentsia’s picture

That second param (or rather knowing when to set it or not, the effects of not setting it when you should and reversedly) sounds a bit worryingly brittle

Agreed. The best answer at this time is: "don't call drupal_render() explicitly once you're in the rendering flow; leave it to Twig to call it via {{ foo }} only". To that end, do we need to call it in:

+++ b/core/modules/node/node.module
@@ -608,14 +607,10 @@ function template_preprocess_node(&$variables) {
+  $variables['date'] = drupal_render($variables['elements']['created'], TRUE);
+  $variables['author_name'] = drupal_render($variables['elements']['uid'], TRUE);

Any reason we can't leave them as render arrays like:

$variables['date'] = $variables['elements']['created'];
$variables['author_name'] = $variables['elements']['uid'];

?

Berdir’s picture

FileSize
66.52 KB
653 bytes

@effulgentsia: Because the output is a translatable string, and that apparently doesn't support passing through render arrays. Just tried it.

Sounds like a good follow-up to fix that, though? Even wondering about just using created/uid directly, right now, we render it even when we later on set display_submitted to FALSE. So maybe we can move all of this into the template (given that we already moved the Submitted by string there too) and treat it similar to links? Once it actually works, I mean.

Looks like we were missing the schema for the boolean_widget...

Status: Needs review » Needs work

The last submitted patch, 188: 2226493-188.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
66.4 KB
2.35 KB

The weight needs to be unique for configurable widgets, or EntityDisplay will re-order and change the default weights.

Status: Needs review » Needs work

The last submitted patch, 190: 2226493-190.patch, failed testing.

yched’s picture

+++ b/core/modules/node/src/NodeForm.php
@@ -129,13 +126,16 @@ public function form(array $form, FormStateInterface $form_state) {
+    // Create the "advanced" vertical tabs before building the form, so that
+    // field widgets may detect its presence and choose to live there.

Still don't get that comment :-) I don't see generic widgets assuming the presence of an "advanced" tabs group in an entity form ?

Berdir’s picture

Status: Needs work » Needs review
FileSize
66.03 KB
2.39 KB

Neither do I :) I find it strange too, to be honest, especially given that seven doesn't even really render that stuff anymore. Removed it again, but left it in that order. I thought it's a comment that was already in HEAD, but that's not the case.

Also, more weight juggling. Weight needs to be unique, above 0 and also not conflict with any dynamic fields (for example, comment has a hardcoded default weight of 20 for the comment field I think).

Fabianx’s picture

I opened #2331393: drupal_render() recursive parameter usage is unclear, safe default should be drupal_render(x) as follow-up to the drupal_render($x) vs drupal_render($x, TRUE) issue.

@Berdir will open a follow-up to ensure twig trans does support render arrays (which is simple todo if we take the performance hit of another function call, for sake of DX we probably should).

Wim Leers’s picture

#174.3/#178:

does this need to be re-evaluated in light of #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render? @Wim ?

No, this is fine :)

(As confirmed by moshe in #177, and further explained by effulgentsia in #182.)

#174.6:

Why don't we just set those as the field labels in the base feild definitions ?

Because "Authored on" doesn't make sense in contexts outside of the node form. I agree with #175.6 that "Created" doesn't make sense, but "Publication date" (which I think was in this patch at some point) would be better. But that doesn't change the fact that "Authored on" only makes sense for the node form.

+++ b/core/modules/node/src/NodeForm.php
@@ -126,8 +126,6 @@ public function form(array $form, FormStateInterface $form_state) {
-    // Create the "advanced" vertical tabs before building the form, so that
-    // field widgets may detect its presence and choose to live there.

I think I might have copy/pasted this from elsewhere in Drupal core when initially working on this patch.

Removing this is probably okay now.


What's left before this can be RTBC? :)

yched’s picture

"Authored on" only makes sense in form contexts

I don't get that. It also works as a label on display (even though node.html.twig used a hardcoded string rather than the field label).

So where doesn't it work ?

Wim Leers’s picture

Ok then, let's change it.

(Months ago this seemed a bad idea, but the way you put it here, it seems fine. It's easy to change later on if we find it to be problematic.)

Berdir’s picture

Which is already the case in the patch now, so as far as I know, there is nothing left to do...

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Yesterday night, #184, yched:

This is RTBC for me when it's green again.

I also did a final round of manual testing just now. Everything works fine, including in-place editing.

Thanks for pushing this to the finish line, Berdir! :)

star-szr’s picture

FileSize
66.03 KB
836 bytes
+++ b/core/modules/node/templates/field--node--created.html.twig
@@ -0,0 +1,18 @@
+<span {{ attributes }}>{{ items }}</span>

+++ b/core/modules/node/templates/field--node--uid.html.twig
@@ -0,0 +1,18 @@
+<span {{ attributes }}>{{ items }}</span>

There shouldn't be a space between <span and {{, Attribute prints its own whitespace so that you don't end up with things like <span > when there are no attributes.

Fixed here because it's removing two characters. No commit credit necessary, this just came across my desk because it's tagged Twig ;)

joelpittet’s picture

FileSize
66.06 KB
2.51 KB

Super minor twig coding standards fixes. Also wanted to get the comments to an even 200:P

  • Indenting docblock.
  • Removing space before printing {{ attributes }} because each attribute has a space before it.

@see https://drupal.org/node/1823416

Crosspost with @Cottser 201:(

Fabianx’s picture

RTBC +1 for #201

Berdir’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This looks great. Just one thing I noticed.

+++ b/core/modules/node/src/NodeForm.php
@@ -233,23 +206,15 @@ public function form(array $form, FormStateInterface $form_state) {
-    $form['promote'] = array(
-      '#type' => 'checkbox',
-      '#title' => t('Promoted to front page'),
-      '#default_value' => $node->isPromoted(),
-      '#group' => 'options',
-      '#access' => $current_user->hasPermission('administer nodes'),
-    );
+    if (isset($form['promote'])) {
+      $form['promote']['#group'] = 'author';
+    }
 
-    $form['sticky'] = array(
-      '#type' => 'checkbox',
-      '#title' => t('Sticky at top of lists'),
-      '#default_value' => $node->isSticky(),
-      '#group' => 'options',
-      '#access' => $current_user->hasPermission('administer nodes'),
-    );
+    if (isset($form['sticky'])) {
+      $form['sticky']['#group'] = 'author';
+    }

Looks like these should be in the options group not author.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
66.06 KB
589 bytes

Ouch, that happened in one of the last changes when cleaning up the form alters, too much copy paste.

Fixed, also visually verified.

Hope it is OK to set this right back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2b0be1d and pushed to 8.0.x. Thanks!

  • alexpott committed 2b0be1d on
    Issue #2226493 by Berdir, Wim Leers, m1r1k, mr.baileys, andypost, scor,...
Wim Leers’s picture

Issue tags: -sprint

Wonderful! :) I just installed D8 and for the very first time, I'm able to in-place edit node author and date. In-place editing of nodes is now finally working as intended!

Wim Leers’s picture

But this did cause a UX regression for changing the author — the UX of the widget is inferior to what we had before. See #2335321: Node author widget UX is lacking (also affects Quick Edit's UX). Still, that's a small price to pay considering the improvements this patch brings, and fixing that UX problem will also benefit all other uses of the entity reference autocomplete widget! :)

EDIT: oops — I linked to this issue :P

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

-    $form['promote'] = array(
-      '#type' => 'checkbox',
-      '#title' => t('Promoted to front page'),
.....
-    $form['sticky'] = array(
-      '#type' => 'checkbox',
-      '#title' => t('Sticky at top of lists'),

A followup to add this removed wording back to the node form is here: #2350013: Descriptions for Promotion Options are too technical

Assuming that was a mistake and the node form wording was not intentionally changed in this issue.