Skip to content
Snippets Groups Projects

Issue #2325899: UI fatal caused by views argument handlers no longer can provide their own default argument handling

Open Issue #2325899: UI fatal caused by views argument handlers no longer can provide their own default argument handling
issue/drupal-2325899:2325899-default-date-argument into 11.x
9 unresolved threads
Open mariana paz requested to merge issue/drupal-2325899:2325899-default-date-argument into 11.x
9 unresolved threads

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
  • mariana paz added 1 commit

    added 1 commit

    Compare with previous version

  • mariana paz
    mariana paz @anairamzap ·
    Author

    Commit 326ef457 Applies the quick changes requested in #177 and the points 4. 5. and 6 from #176

    From that last comment/review point 1 seems to collide with what's mentioned in 3. So I think the only missing change request is the one outlined in point 2, regarding Node cache:

    +++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
@@ -0,0 +1,111 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getCacheMaxAge() {
+    return Cache::PERMANENT;
+  }

+++ b/core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
@@ -0,0 +1,111 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getCacheMaxAge() {
+    return Cache::PERMANENT;
+  }

    Are we should about this - I would have thought that it would be best add the node as a cacheable dependency - I think we need to vary by the node's cache tags for example.

    I checked the node ViewsArgumentDefault (core/modules/node/src/Plugin/views/argument_default/Node.php) and the getCacheMaxAge() method is also using Cache::PERMANENT so I'm really not sure if we should do something different here on the node_changed and node_created plugins.

    And finally, I though it was better to work on a MR for 10.1.x and I will try to provide a proper re-roll for 9.5 as a patch.

  • mariana paz added 47 commits

    added 47 commits

    Compare with previous version

    Toggle commit list
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
core/modules/node/src/Plugin/views/argument_default/NodeChanged.php 0 → 100644
23 */
24 class NodeChanged extends ArgumentDefaultPluginBase implements CacheableDependencyInterface {
25
26 /**
27 * The route match.
28 *
29 * @var \Drupal\Core\Routing\RouteMatchInterface
30 */
31 protected $routeMatch;
32
33 /**
34 * The date formatter service.
35 *
36 * @var \Drupal\Core\Datetime\DateFormatterInterface
37 */
38 protected $dateFormatter;
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
    • core/modules/node/src/Plugin/views/argument_default/NodeChanged.php 0 → 100644
    49 * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
    50 * The route match.
    51 * @param \Drupal\Core\Datetime\DateFormatterInterface $date_formatter
    52 * The date formatter service.
    53 */
    54 public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteMatchInterface $route_match, DateFormatterInterface $date_formatter) {
    55 parent::__construct($configuration, $plugin_id, $plugin_definition);
    56
    57 $this->routeMatch = $route_match;
    58 $this->dateFormatter = $date_formatter;
    59 }
    60
    61 /**
    62 * {@inheritdoc}
    63 */
    64 public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
    • core/modules/node/src/Plugin/views/argument_default/NodeChanged.php 0 → 100644
    62 * {@inheritdoc}
    63 */
    64 public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    65 return new static(
    66 $configuration,
    67 $plugin_id,
    68 $plugin_definition,
    69 $container->get('current_route_match'),
    70 $container->get('date.formatter')
    71 );
    72 }
    73
    74 /**
    75 * {@inheritdoc}
    76 */
    77 public function getArgument() {
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
    • core/modules/node/src/Plugin/views/argument_default/NodeChanged.php 0 → 100644
    79 if (($node = $this->routeMatch->getParameter('node')) && $node instanceof NodeInterface) {
    80
    81 // The Date argument handlers provide their own format strings, otherwise
    82 // use a default.
    83 $format = $this->argument instanceof Date ? $this->argument->getArgFormat() : 'Y-m-d';
    84
    85 return $this->dateFormatter->format($node->getChangedTime(), 'custom', $format);
    86 }
    87 return FALSE;
    88 }
    89
    90 /**
    91 * {@inheritdoc}
    92 */
    93 public function getCacheContexts() {
    94 return ['url'];
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
    • core/modules/node/src/Plugin/views/argument_default/NodeCreated.php 0 → 100644
    40 /**
    41 * Constructs a new NodeCreated instance.
    42 *
    43 * @param array $configuration
    44 * A configuration array containing information about the plugin instance.
    45 * @param string $plugin_id
    46 * The plugin_id for the plugin instance.
    47 * @param mixed $plugin_definition
    48 * The plugin implementation definition.
    49 * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
    50 * The route match.
    51 * @param \Drupal\Core\Datetime\DateFormatterInterface $date_formatter
    52 * The date formatter service.
    53 */
    54 public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteMatchInterface $route_match, DateFormatterInterface $date_formatter) {
    55 parent::__construct($configuration, $plugin_id, $plugin_definition);
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
    • core/modules/node/src/Plugin/views/argument_default/NodeCreated.php 0 → 100644
    70 $container->get('date.formatter')
    71 );
    72 }
    73
    74 /**
    75 * {@inheritdoc}
    76 */
    77 public function getArgument() {
    78 // Return the current node creation time if a current node can be found.
    79 if (($node = $this->routeMatch->getParameter('node')) && $node instanceof NodeInterface) {
    80
    81 // The Date argument handlers provide their own format strings, otherwise
    82 // use a default.
    83 $format = $this->argument instanceof Date ? $this->argument->getArgFormat() : 'Y-m-d';
    84
    85 return $this->dateFormatter->format($node->getCreatedTime(), 'custom', $format);
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/modules/node/tests/src/Functional/Views/DateArgumentDefaultTest.php 0 → 100644
    157
    158 // Update the View to use the Y-m format argument.
    159 $view = View::load('test_argument_node_date');
    160 $display = &$view->getDisplay('block_2');
    161 $display['display_options']['arguments']['changed']['plugin_id'] = 'date_year_month';
    162 $display['display_options']['arguments']['changed']['field'] = 'changed_year_month';
    163 $display['display_options']['arguments']['changed']['id'] = 'changed_year_month';
    164 $view->save();
    165
    166 // Test that the nodes with a changed date in the same month are shown.
    167 $this->drupalGet($this->fixedTimeNode->toUrl());
    168 $assert->pageTextContains($this->fixedTimeNode->getTitle());
    169 $assert->pageTextContains($this->sameMonthNode->getTitle());
    170 $assert->pageTextNotContains($this->currentNode->getTitle());
    171
    172 // Update the View to use the date format argument for non-date field.
    • Comment on lines +167 to +172
      Lee Rowlands
      Lee Rowlands @larowlan ·
      Maintainer

      Yeah we're missing coverage here of what happens if you update the node so that its changed time updates, does the view invalidate the render cache?

    • Len Swaneveld
      Len Swaneveld @Lendude ·
      Developer

      Isn't that handled by adding the entity list cache tag (i.e. node_list in this case) to all relevant Views?

    • Please register or sign in to reply
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/modules/views/src/Plugin/views/argument/Date.php
    140 140 return parent::getFormula();
    141 141 }
    142 142
    143 /**
    144 * Returns the date format used in the query in a form usable by PHP.
    145 *
    146 * @return string
    147 * The date format used in the query.
    148 */
    149 public function getArgFormat(): string {
    • Lee Rowlands
      Lee Rowlands @larowlan ·
      Maintainer

      Can we make this getArgumentFormat - we should be using complete words for APIs

    • Len Swaneveld
      Len Swaneveld @Lendude ·
      Developer

      Since the property this gets is called argFormat, I'm not sure this would make things clearer in this case.

      Should the property be called 'argumentFormat'? Probably, but that seems out of scope.

    • Please register or sign in to reply
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
    • core/modules/views/src/Plugin/views/argument_default/Date.php 0 → 100644
    47 * @param array $configuration
    48 * A configuration array containing information about the plugin instance.
    49 * @param string $plugin_id
    50 * The plugin_id for the plugin instance.
    51 * @param mixed $plugin_definition
    52 * The plugin implementation definition.
    53 * @param \Drupal\Core\Datetime\DateFormatterInterface $date_formatter
    54 * The date formatter service.
    55 * @param \Drupal\Component\Datetime\TimeInterface $time
    56 * The time service.
    57 */
    58 public function __construct(array $configuration, $plugin_id, $plugin_definition, DateFormatterInterface $date_formatter, TimeInterface $time) {
    59 parent::__construct($configuration, $plugin_id, $plugin_definition);
    60
    61 $this->dateFormatter = $date_formatter;
    62 $this->time = $time;
  • Lee Rowlands
    Lee Rowlands @larowlan ·
    Maintainer

    Thanks folks, a few things

  • Len Swaneveld added 1 commit

    added 1 commit

    • 2e5f61ae - Use constructor property promotion

    Compare with previous version

  • Len Swaneveld added 1 commit

    added 1 commit

    • d4fc4a15 - Add base class for node date argument defaults

    Compare with previous version

  • Len Swaneveld added 1 commit

    added 1 commit

    Compare with previous version

    • Please register or sign in to reply