Problem/Motivation

This is a partial extract from views.view.content_recent.yml.

id: content_recent
label: 'Recent content'
...
display:
  default:
    display_plugin: default
    id: default
    display_title: Master
    position: 0
    display_options:
      ...
      link_display: custom_url
      ...
  block_1:
    ...
    display_options:
      link_url: admin/content
      ...

Even though the link_display property is set for the default display, it does not load because link_url is not set. That property is set for the block but since that does not define link_display and/or does not override it, and is never used as seen from the screenshot below:

Further, this is blocking another critical issue: #2409209: Replace all _url() calls beside the one in _l()

Proposed resolution

Modify the view to store the link_url for the default view in the Recent Content view and any other view that suffers from this.

Remaining tasks

Write a patch

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the functionality does not work as seemingly intended.
Issue priority Critical because this blocks another critical issue: #2409209: Replace all _url() calls beside the one in _l()
Disruption Not disruptive
CommentFileSizeAuthor
#15 vdc-2418163-15-PASS.patch6.97 KBxjm
#15 vdc-2418163-FAIL.patch1.23 KBxjm
#14 patch.png27.16 KBxjm
#14 head.png16.04 KBxjm
#11 interdiff-label.txt1.01 KBxjm
#11 vdc-2418163-11-view-only.patch5.73 KBxjm
#9 vdc-2418163-9-view-only.patch5.9 KBxjm
#3 screenshot-view-custom-url-fix.png11.38 KBhussainweb
#1 custom_url_is_not_set-2418163-1.patch567 byteshussainweb
screenshot-view-custom-url.png12.28 KBhussainweb

Comments

hussainweb’s picture

hussainweb
Primary language English
commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs manual testing, +Needs screenshots
StatusFileSize
new custom_url_is_not_set-2418163-1.patch567 bytes

Simple fix. This would need screenshots.

Also, I was a little confused when I said views.view.user_admin_people.yml also needs fixing. The link_display points to another display and not custom_url. This is the only view that uses a custom url in core, apparently. Updating issue summary.

hussainweb’s picture

hussainweb
Primary language English
commented
Issue tags: +Novice
hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new screenshot-view-custom-url-fix.png11.38 KB

The patch works. Here is the screenshot from simplytest.me:

hussainweb’s picture

hussainweb
Primary language English
commented
Issue tags: -Needs manual testing, -Needs screenshots +Quickfix

Updating tags. :)

hussainweb’s picture

hussainweb
Primary language English
commented

Even though it looks like the fix works, I am wondering if we should clean up the views files further. Should we continue to have link_url under block_1 as well or can we remove it? Exporting the view also adds a property called display_extenders but I think that will become too much to change here (and we might have to change all other yml files as well). We should target to get this done soon so that the other critical can move ahead.

We can also merge the fix in that issue but I wanted to keep the discussion separate. It really isn't a duplicate issue either, as this bug had different effects unrelated to the issue in #2409209: Replace all _url() calls beside the one in _l().

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented

I think this is a general problem with the view definition, but I am not sure the patch is quite correct. The next step is to manually make the same view, and export it to see what is different.

My initial thought is that the `link_url` needs to be removed from the block display, too.

We also need to see why this wasn't caught by an existing test and/or add test coverage for it.

dawehner’s picture

dawehner
Primary language German
commented

I'd agree.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: Unassigned » xjm
Issue tags: -Novice, -Quickfix

Looking into this.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +VDC, +D8 Accelerate NJ
StatusFileSize
new vdc-2418163-9-view-only.patch5.9 KB

So I rebuilt the view through the UI (attached) and then went over the diff with the default view in HEAD. Attached is that patch. I'll go over the individual changes in a subsequent comment.

xjm’s picture

xjm
she/her
Primary language English
commented 
  1. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -434,24 +408,24 @@ display:
       use_more: true
       use_more_always: true
       use_more_text: More
+      link_url: admin/content
       link_display: custom_url
...
     display_options:
-      link_url: admin/content

    This is the source of the bug: For some reason, the link_url existed on the view under the display options for the block display rather than the default display, separately from the rest of the more link configuration. So, I'm going to test and see what happens when I override this on a display. 

  2. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -111,54 +103,36 @@ display:
-            more_link: false
-            more_link_text: ''
-            more_link_path: ''

    Maybe this is related to the bug? 

  3. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -62,7 +62,6 @@ display:
-            timestamp: title

@@ -74,13 +73,6 @@ display:
-            timestamp:
-              sortable: false
-              default_sort_order: asc
-              align: ''
-              separator: ''
-              empty_column: false
-              responsive: ''

    Legacy field configuration that I've confirmed goes away with the existing view if I re-save the style plugin configuration form. 

  4. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -111,54 +103,36 @@ display:
-          relationship: none
-          group_type: group
-          admin_label: ''
...
-          exclude: false
...
+          relationship: none
+          group_type: group
+          admin_label: ''
...
+          exclude: false

    Moved lines. 

  5. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -111,54 +103,36 @@ display:
-          label: ''
...
+          label: Title
...
-          element_label_colon: false
+          element_label_colon: true

    Me accidentally not removing the label from this field; I'll fix that in a subsequent patch. 

  6. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -111,54 +103,36 @@ display:
+          entity_type: node
+          entity_field: title
...
-          entity_type: node
-          entity_field: title

    Moved lines. 

  7. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -111,54 +103,36 @@ display:
-            external: false
-            replace_spaces: false
-            path_case: none
-            trim_whitespace: false
-            alt: ''
-            rel: ''
-            link_class: ''
-            prefix: ''
-            suffix: ''
-            target: ''
-            nl2br: false
-            max_length: ''
...
-            preserve_tags: ''

    No idea what said this originally but it's all empty configuration so no effect on the view. 

  8. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -111,54 +103,36 @@ display:
+            trim: false
...
-            trim: false

    Moved lines. 

  9. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -111,54 +103,36 @@ display:
+          hide_empty: false
+          empty_zero: false
+          link_to_node: true
+          plugin_id: node
...
-          hide_empty: false
-          empty_zero: false
...
-          link_to_node: true
-          plugin_id: node

    Moved lines. 

  10. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -211,9 +185,9 @@ display:
-          plugin_id: user_name
...
+          plugin_id: user_name

    Moved line. 

  11. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -262,9 +236,9 @@ display:
-          text: edit
...
+          text: Edit

@@ -313,9 +287,9 @@ display:
-          text: delete
...
+          text: Delete

    Me following our UI text standards in the UI; technically a minor bug in HEAD. 

  12. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -262,9 +236,9 @@ display:
-          plugin_id: node_link_edit
...
+          plugin_id: node_link_edit

    Moved line. 

  13. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -313,9 +287,9 @@ display:
-          plugin_id: node_link_delete
...
+          plugin_id: node_link_delete

    Moved line. 

  14. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -329,7 +303,7 @@ display:
-            operator_id: '0'
+            operator_id: ''

    Different (presumably schema-updated) value of false. 

  15. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -352,8 +326,8 @@ display:
-          plugin_id: node_status
...
+          plugin_id: node_status

    Moved line. 

  16. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -391,9 +365,9 @@ display:
-          plugin_id: language
...
+          plugin_id: language

    Moved line. 

  17. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -407,9 +381,9 @@ display:
-          plugin_id: date
...
+          plugin_id: date

    Moved line. 

  18. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -434,24 +408,24 @@ display:
-      filter_groups:
-        operator: AND
-        groups: {  }

    This is the empty value that would be saved if it previously had a different filter configuration, but has no effect because it is the default behavior. 

  19. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -434,24 +408,24 @@ display:
+      display_extenders: {  }
...
+      display_extenders: {  }

    This line also was on the block display options instead of the master display, but it's empty in any case. 

  20. +++ b/core/modules/node/config/install/views.view.content_recent.yml
@@ -434,24 +408,24 @@ display:
+      field_langcode: '***LANGUAGE_language_content***'
+      field_langcode_add_to_query: null
...
-      field_langcode: '***LANGUAGE_language_content***'
-      field_langcode_add_to_query: null

    Moved lines.

xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new vdc-2418163-11-view-only.patch5.73 KB
new interdiff-label.txt1.01 KB

Just fixing the label thing.

xjm’s picture

xjm
she/her
Primary language English
commented

So looks like this is the only view in HEAD that has a more link, other than the test view for that feature:

[mandelbrot:core | Sun 18:32:21] $ grep -r "use_more:" *
modules/node/config/install/views.view.content_recent.yml:      use_more: true
modules/user/config/install/views.view.user_admin_people.yml:      use_more: false
modules/views/config/schema/views.data_types.schema.yml:        use_more:
modules/views/config/schema/views.data_types.schema.yml:    use_more:
modules/views/tests/modules/views_test_config/test_views/views.view.test_display_more.yml:      use_more: true

I confirmed that the more_link bits are actually unrelated; this is an option in the field rewrite settings to add the more link if the content is truncated.

Status: Needs review » Needs work

The last submitted patch, 11: vdc-2418163-11-view-only.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new head.png16.04 KB
new patch.png27.16 KB

So I have tried lots of variations of adding and removing overrides in the UI and cannot reproduce what's in HEAD that way.

The more link feature itself has test coverage (as above). The block has test coverage in NodeBlockFunctionalTest, but it doesn't test the more link. Adding test coverage.

Attached screenshots show the actual block in HEAD and with the patch.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: xjm » Unassigned
Status: Needs work » Needs review
StatusFileSize
new vdc-2418163-FAIL.patch1.23 KB
new vdc-2418163-15-PASS.patch6.97 KB

Now with tests.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Seems legit.

xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#2053015: Add user access checks to 'more' link, +#2409209: Replace all _url() calls beside the one in _l()

The last submitted patch, 15: vdc-2418163-FAIL.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Critical » Major

Oh, I should note, this is not critical in itself presently -- but the malformed view blocks us in #2409209: Replace all _url() calls beside the one in _l() because it breaks entirely with that conversion.

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Major » Critical

Er. So per #19. :)

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Custom URL is not set for views using custom link display » Recent content view "more" link configuration is malformed
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Great work! I had a few questions but #10 pre-emptively answered all of them.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 24bfa75 on 8.0.x 
    Issue #2418163 by xjm, hussainweb: Recent content view "more" link...

Status: Fixed » Closed (fixed)

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