Problem/Motivation

Like for too many other pages we are returning raw html when it would be really easy to return instead a renderable array.

This way other modules could just alter it instead of overriding the full page (with menu_alter()) and we would save a lot of clashes between contrib modules.

Some modules overriding this page (and then clashing) are: Panels, Internationalization.

Related #1431142: Name (add semantic indexes) for rows and columns for translation node overview table so other modules can alter content.

Proposed resolution

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary noting if allowed during the beta Instructions

User interface changes

API changes

CommentFileSizeAuthor
#25 properly_build_page-1431168-25.patch3.73 KBalansaviolobo
#22 node_build_pages_1431168-22.patch2.94 KBinternetdevels
#19 1431168_node_build_pages_19.patch4.49 KBdavid.lukac
#19 interdiff-1431168-19.txt575 bytesdavid.lukac
#16 1431168_node_build_pages_16.patch4.48 KBdeepakaryan1988
#11 1431168_node_build_pages_11.patch3.97 KBdawehner
#10 1431168_node_build_pages_8-10.patch3.96 KBdas-peter
#8 1431168_node_build_pages_8.patch3.69 KBdawehner
#7 1431168_node_build_pages_7.patch3.5 KBdawehner
#5 1431168_node_build_pages_5.patch3.14 KBjose reyero
#3 1431168_node_build_pages-d8.patch3.14 KBjose reyero
node_add_list-build_page.patch488 bytesjose reyero

Comments

jose reyero’s picture

jose reyero commented
Title: Properly build page content so other modules can alter it. » Properly build page content for node_add_page() so other modules can alter it.

Better title.

casey’s picture

casey commented
Version: 7.x-dev » 8.x-dev

I agree, but we are developing on the D8 branch.

jose reyero’s picture

jose reyero commented
StatusFileSize
new 1431168_node_build_pages-d8.patch3.14 KB

Ok, updated patch for D8.

And done the same to node_revision_overview() page.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work
Issue tags: +D8MI, +language-content

Looks like a great idea! Do we have any test coverage for this page to ensure it works?

@Jose: please submit patches without the -dX ending, so they actually get tested. The -dX ending makes the patch bypass testing.

jose reyero’s picture

jose reyero commented
Status: Needs work » Needs review
StatusFileSize
new 1431168_node_build_pages_5.patch3.14 KB

Updated and renamed patch.
About the test coverage, since existing pages shouldn't change, existing tests will do

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/node/node.pages.incundefined
@@ -618,22 +623,22 @@ function node_revision_overview($node) {
                                . (($revision->log != '') ? '<p class="revision-log">' . filter_xss($revision->log) . '</p>' : ''),
                      'class' => array('revision-current'));

Let's normalize a indent for this lines too

+++ b/core/modules/node/node.pages.incundefined
@@ -618,22 +623,22 @@ function node_revision_overview($node) {
-        $operations[] = l(t('revert'), "node/$node->nid/revisions/$revision->vid/revert");
+        $operations['revert'] = l(t('revert'), "node/$node->nid/revisions/$revision->vid/revert");
       }
       if ($delete_permission) {
-        $operations[] = l(t('delete'), "node/$node->nid/revisions/$revision->vid/delete");
+        $operations['delete'] = l(t('delete'), "node/$node->nid/revisions/$revision->vid/delete");
       }
     }
-    $rows[] = array_merge($row, $operations);
+    $rows[$revision->vid] = array_merge($row, $operations);

Do we have a formal conclusion about operations in tables? Some core's modules places then in one cell some in different. As I remember there was a troubles within field's table when i18n enbaled.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 1431168_node_build_pages_7.patch3.5 KB

There is an issue about implementing a ui pattern for operation links: #1480854: Convert operation links to '#type' => 'operations'

Do we have a formal conclusion about operations in tables? Some core's modules places then in one cell some in different. As I remember there was a troubles within field's table when i18n enabled.
There seems to be no real conclusion about that.

There are places #theme 'links' is used. Here using actual keys gives you automatic helpful css classes which is kind of cool.
There are places like here on which just hard-coded links are used, which are hard to alter.

Personally i would preper to have some alterable code, so for example using #theme links would be the way to go. What do you think?

This patch is just about fixing the indentation problem.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 1431168_node_build_pages_8.patch3.69 KB

Here is a new patch which converts it to '#type' => 'link'

jose reyero’s picture

jose reyero commented

Improvements by @dereine look very good to me. About 'operation links' I've posted my comment there but anyway I don't think we need to wait for them to decide as whatever it is needed there, this patch will just ease the path for that.

das-peter’s picture

das-peter commented
StatusFileSize
new 1431168_node_build_pages_8-10.patch3.96 KB

Just gave this a review and changed parts of the code to apply to the coding standards.
Unfortunately there's still following construct that looks really odd even if it complies to the standards:

    if ($revision->current_vid > 0) {
      $row['revision'] = array(
        'data' => t(
          '!date by !username',
...
dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 1431168_node_build_pages_11.patch3.97 KB

Some improvements for the uglyness.

das-peter’s picture

das-peter commented

I'm absolutely fine with that. Now you've at least a small change to understand that :P

jair’s picture

jair commented
Issue tags: +Needs reroll

Needs reroll

jair’s picture

jair commented

Needs reroll

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

.

deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
commented
Status: Needs work » Needs review
StatusFileSize
new 1431168_node_build_pages_16.patch4.48 KB

Re-rolling patch from #11

Status: Needs review » Needs work

The last submitted patch, 1431168_node_build_pages_16.patch, failed testing.

david.lukac’s picture

david.lukac commented
Assigned: Unassigned » david.lukac

Assigning to me during DrupalCon Prague sprint.

david.lukac’s picture

david.lukac commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-1431168-19.txt575 bytes
new 1431168_node_build_pages_19.patch4.49 KB

Revision timestamp coming from node_revision_list() was referenced as $revision->timestamp. Changing to $revision->revision_timestamp.

The last submitted patch, 1431168_node_build_pages_19.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Component: node.module » node system
Issue summary: View changes

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

internetdevels’s picture

internetdevels commented
StatusFileSize
new node_build_pages_1431168-22.patch2.94 KB

Updated patch for Drupal 8

oleksandr senenko’s picture

oleksandr senenko commented
Status: Needs work » Needs review
yesct’s picture

yesct commented
Issue tags: -Needs reroll

patch applies to d8 head. removing needs reroll tag.

alansaviolobo’s picture

alansaviolobo commented
StatusFileSize
new properly_build_page-1431168-25.patch3.73 KB

reroll

Status: Needs review » Needs work

The last submitted patch, 25: properly_build_page-1431168-25.patch, failed testing.

yesct’s picture

yesct commented
Assigned: david.lukac » Unassigned
Issue summary: View changes
Issue tags: +Needs issue summary update

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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.

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

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley commented
Status: Needs work » Closed (outdated)

It looks like NodeController::addPage is already doing this.

The Revision routes will be covered in #3153559: Switch Node revision UI to generic UI if not already.

Closing as outdated.