Problem/Motivation

Part of #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() effort for removing drupal_static() from node_mark().

Proposed resolution

Remove drupal_static() from node_mark(). As node_mark() relies on history_read_multiple() and that already uses static cache this layer of cache brings no benefit.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#13 interdiff_8-13.txt1.68 KBvsujeetkumar
#13 3037202-13.patch5.96 KBvsujeetkumar
#8 3037202-8.patch5.98 KBhardik_patel_12
#5 3037202-5.patch6 KBclaudiu.cristea
#5 3037202-5.interdiff.txt1.82 KBclaudiu.cristea
#2 3037202-2.patch6 KBclaudiu.cristea

Issue fork drupal-3037202

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

  • 9.3.x Comparecompare
    • 1 hidden branch
  • 3037202-nodemark Comparechanges, plain diff MR !846
About issue forks

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3037202-2.patch6 KB

Patch.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

martin107’s picture

martin107 commented

This looks good to me

as a minor point only

messages of the form "deprecated in Drupal 8.7.0 " should be incremented to 8.8.0

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 3037202-5.interdiff.txt1.82 KB
new 3037202-5.patch6 KB

Fixed in patch and in CR.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

hardik_patel_12’s picture

hardik_patel_12 commented
StatusFileSize
new 3037202-8.patch5.98 KB

Re-rolled against 9.1.x-dev.Kindly review a new patch.

martin107’s picture

martin107 commented

Sorry time has advanced.

"deprecated in Drupal 8.8.0 " should be incremented to 9.1.0"

and also changed to

"and will be removed from Drupal 10"

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Needs work to update the deprecation.

vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new 3037202-13.patch5.96 KB
new interdiff_8-13.txt1.68 KB

Updated deprecation, Please have a look.

claudiu.cristea opened merge request !846

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work 
+++ b/core/modules/node/node.module
@@ -184,10 +184,16 @@ function node_title_list(StatementInterface $result, $title = NULL) {
+  @trigger_error("node_mark() is deprecated in Drupal 9.3.0 and will be removed before Drupal 10.0.0. There's no replacement for this function. See https://www.drupal.org/node/3037203.", E_USER_DEPRECATED);

This is the wrong format, it should say "drupal:9.3.0" and "drupal:10.0.0" for the version numbers. The tests will need updating too. I think you also need to use $this->expectDeprecation() instead of the annotation in the test.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

Sorry, I think we've worked in parallel. The MR also contains #15 and more.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Also I wonder if we should not move/deprecate MARK_READ, MARK_NEW & MARK_UPDATED constants in this MR. I think yes.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Agree we should deprecate the constants at the same time. There is an unrelated change to a list() call (PHPStorm does this by itself, annoyingly), otherwise I think this is good to go.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Hiding patches as we are using the MR workflow now.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

In MR since #8:

  • Updated the Drupal version in deprecation messages.
  • Applied the standard pattern to messages.
  • Updated the change notice.
  • Converted the test to a kernel test, avoiding container trick.
  • Using $this->expectDeprecation() instead of annotation.
  • Deprecated global MARK_NEW, MARK_UPDATED & MARK_READ constants
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Thinking again, should all this functionality belong to history.module? It only works when history is enabled - decoupling that is out of scope for this issue, but should the constants belong over there somewhere? If so maybe leave them where they are and defer that to a followup?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@longwave, ref. #21:

(...) should all this functionality belong to history.module?

Yes, it should be there. I was thinking more, trying to understand what a "mark" is:

  • Mark is not part of any module, it's provided in core as template, see theme.inc and mark.html.twig. That means a 3rd party module can use them for a custom entity type. For this reason I think the constants, the theme definition (see drupal_common_theme() and template files (mark.twig.html) should live in core. We need to revert the constants deprecation.
  • In Drupal core, the mark is only applied to nodes via History module.

Issues with moving node_mark() to History module:

  • Normally, the History module, should intercept the render array rows produced by NodeListBuilder and just add the mark. However, this seems not possible right now. I don't see any path where the rows produced by an entity list builder handler can be altered. Of, course, there's always the possibility to swap the Node list builder handler class. But this has a cost when multiple modules are doing this: only one of them will win. I've opened #3221351: 3rd-party should be able to alter the EntityListBuilder rows to properly fix this.

Fixes:

  1. Reverted the move of constants.
  2. Move the logic to History module rather than NodeListBuilder.
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Title: Move node_mark() functionality in NodeListBuilder. Deprecate node_mark() » Deprecate node_mark() and move its functionality in History module
Issue summary: View changes

Updated title & IS

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Title: Deprecate node_mark() and move its functionality in History module » Remove drupal_static() from node_mark()
Issue summary: View changes
Status: Needs work » Needs review

Went with the approach suggested by @Berdir

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Reviewed the MR.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Reviewed & tested by the community

> @berdir, well, I didn't intend to go with the full refactoring path but I was afraid that simply removing the node_mark() static cache or make it static internal will not satisfy.

I satisfies me :)

As mentioned in the MR, given that history_read_multiple() has a static cache, the performance gain would be absolutely tiny, and considering that this is used only in a single place in core, I think this is fine.

Don't need to untangle the interdependencies between history.module and node.module in this issue.

  • catch committed 0e9bec3 on 9.3.x 
    Issue #3037202 by claudiu.cristea, vsujeetkumar, Hardik_Patel_12,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Agreed this is enough here. It's likely the static cache was added to the history function when history was factored out into a module, but just not removed from node_mark at the same time. Opened #3223211: Deprecate node_mark for killing off node_mark() altogether.

Committed 0e9bec3 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

Published the CR