Skip to content
Snippets Groups Projects

Resolve #3483304 Allow disabling page displays for node types

Open Resolve #3483304 Allow disabling page displays for node types
5 unresolved threads
Open Lee Rowlands requested to merge issue/drupal-3483304:3483304-add-support-for into 11.x
5 unresolved threads

Closes #3483304

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Lee Rowlands
  • Lee Rowlands added 1 commit

    added 1 commit

    Compare with previous version

  • Lee Rowlands added 1 commit

    added 1 commit

    Compare with previous version

  • Lee Rowlands resolved all threads

    resolved all threads

  • Lee Rowlands added 4 commits

    added 4 commits

    Compare with previous version

  • Lee Rowlands added 1 commit
  • Lee Rowlands added 1 commit
  • Lee Rowlands added 3 commits

    added 3 commits

    Compare with previous version

  • Lee Rowlands added 3 commits

    added 3 commits

    Compare with previous version

  • 117 117 $this->submitForm($edit, 'Save (this translation)');
    118 118 $edit = [$this->fieldName . '[0][alt]' => 'Scarlett Johansson image', $this->fieldName . '[0][title]' => 'Scarlett Johansson image title'];
    119 119 $this->submitForm($edit, 'Save (this translation)');
    120 $this->drupalGet('/node/' . $default_language_node->id());
    • Author Maintainer

      In HEAD we use $form_state->setRedirect('entity.node.canonical', ['node' => $node->id()]) which doesn't take into account the language whilst $form_state->setRedirectUrl($node->toUrl()) takes care to set $options['language']

    • Please register or sign in to reply
  • Lee Rowlands added 1 commit

    added 1 commit

    • 3931297c - Remove deprecation, no nice way to do it

    Compare with previous version

  • Author Maintainer

    I backed out the deprecation as it is triggered in update path tests, don't know that there's a nice way to handle it

  • catch @catch started a thread on the diff
  • 27 35 '_permission' => 'access content overview',
    28 36 ]);
    29 37 }
    38
    39 // @todo Move this to a subscriber in the Drupal\Core\Routing namespace
    40 // https://www.drupal.org/project/drupal/issues/3484255
    • Maintainer

      Not sure why https://www.drupal.org/project/drupal/issues/3484255 is the follow-up for making this generic to different entity types when that issue is adding a feature. Two questions I guess:

      1. Is it complex to implement it for all entity types here?

      2. Should there be a different follow-up issue for this if the answer to #1 is 'yes'?

    • Author Maintainer

      I will try to round on that today to at least validate the work here. I don't want this to paint us into a corner if it takes a while to progress other entity-types or displays

    • Please register or sign in to reply
  • Lee Rowlands added 23 commits

    added 23 commits

    Compare with previous version

  • 22 protected EntityFieldManagerInterface $entityFieldManager,
    23 protected EntityDisplayRepositoryInterface $entityDisplayRepository,
    24 protected RouteBuilderInterface $routeBuilder,
    25 ) {
    34 26 }
    35 27
    36 28 /**
    37 29 * {@inheritdoc}
    38 30 */
    39 public static function create(ContainerInterface $container) {
    31 public static function create(ContainerInterface $container): static {
    40 32 return new static(
    41 $container->get('entity_field.manager')
    33 $container->get(EntityFieldManagerInterface::class),
    34 $container->get(EntityDisplayRepositoryInterface::class),
    35 $container->get(RouteBuilderInterface::class),
  • 178 172 '#title' => $this->t('Display settings'),
    179 173 '#group' => 'additional_settings',
    180 174 ];
    175 $full_display = NULL;
    176 if (!$type->isNew()) {
    177 $full_display = $this->entityDisplayRepository->getViewDisplay('node', $type->id(), 'full');
    178 }
    179 $form['display']['page_display'] = [
    180 // @todo Move this to the entity view display edit form in
    181 // https://www.drupal.org/project/drupal/issues/3484255
    182 '#type' => 'checkbox',
    183 '#title' => $this->t('Create page display'),
    184 '#default_value' => $full_display === NULL || $full_display->hasPageDisplay() || $full_display->isNew(),
    185 '#description' => $this->t('Uncheck this to prevent the content-type from having a full page display.'),
    • Nit: Mostly we see content type rather than content-type unless it's referring to the HTTP header.

      Suggested change
      185 '#description' => $this->t('Uncheck this to prevent the content-type from having a full page display.'),
      185 '#description' => $this->t('Uncheck this to prevent the content type from having a full page display.'),
    • Also it seems backwards to describe the unchecked behaviour rather than the checked behaviour, even if checked is the default.

    • Please register or sign in to reply
    • Had a bit of a read through the MR. My main concern is with the term "page display". I don't really know what that means, but I also don't have a better suggestion. We already have view display and form display but it seems that page display is something that applies to view displays.

      Have left a couple other remarks.

    • Agree that the terminology is challenging and "page display" is confusing to introduce here. We have the same concept for media and the setting is called "Standalone media URL" but it's different because in that case, the default is off and the edge case is on. "Standalone node URL" really does not sound right! But that's also why the checkbox is hard because it's easier to explain what happens when you uncheck the box.

      Basically I have no suggestions, just noting this needs more thought.

    • Please register or sign in to reply
  • Lee Rowlands added 60 commits

    added 60 commits

    Compare with previous version

  • Please register or sign in to reply
    Loading