In the interest of improved cooperation with other modules that attempt to set the page title, e.g. Metatag, Page Title, etc, provide a way to inform Views to not assign the page title, e.g. using the Block module's standard '<none>' string.

Original request:

If the Title field of a view is set to an empty string, Views will still assign that to the page title which can overwrite values set by other modules. Views should first verify if the page title is not empty and only assign it if it is not empty.

CommentFileSizeAuthor
#9 views-n1760494-9.patch2.5 KBDamienMcKenna
#2 views-n1760494-d7.patch665 bytesDamienMcKenna
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Category: feature » bug
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Active » Needs review
FileSize
views-n1760494-d7.patch665 bytes

This patch only runs drupal_set_title() if the title is not empty.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Actually, an empty title is the intent. Otherwise, how do you give a page no title at all?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

This will (potentially) interfere with Metatag, but I'm still working on testing it: #1151936: Ensure Metatag works OOTB using Views to display Drupal core's entities

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

Maybe there needs to be an option to not set the title?

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

I concur with DamienMcKenna. Either this patch should be applied, or there should be a checkbox on the TItle dialog (disabled by default) that says something along the lines of "Give this page a blank title". That way there's an explicit difference between not setting a title, and specifying that there shouldn't be one.

I was originally going to post exactly the same patch as #2, so I'm glad I found this issue. This is a major problem because if it isn't remedied (either by the proposed patch, or adding an option), then other modules can never change the title of a Views Page. This means that if you need a custom title which can't be generated by the title override options that Views provides, you're SOL.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

I'd say 90% of the time if the title field is empty the intent of the site builder is for views to do nothing, so the thrust of this patch makes sense to me.

I could see using something like "" to indicate that the user really means "".

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

Another option would be to support the core practice of using "<none>" to indicate to not assign a title?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
FileSize
views-n1760494-9.patch2.5 KB

How about something like this? It doesn't run drupal_set_title() if the title field is '<none>', tweaks the logic for block display to do the same thing, and has an update on the title field's description to indicate the new usage option.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I'm not actually entirely sure I agree with this.

If you create a page display, then that display expects to actually be the primary arbiter of the page. That is the whole purpose of a page display.

I have had instances where crappy code does a drupal_set_title in the background of a view (most often in node_view hooks) and if I want the page to have no title (like on the frontpage), then this patch would break my page until I went in and manually intervened.

I'm trying to figure out in what circumstances some kind of alter should have the authority to go in and say that the title on this view is wrong, let me correct it; and if so, shouldn't it just actually tell the view what the title should be?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

The purpose of the patch was to provide a new feature for not outputting the title at all, thus providing a clear direction that would avoid the original confusion.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

At the same time, I must mention that modules like Page Title, Metatag and others to provide methods for overriding the page's title using either per-entity or per-path options that could collide with Views (or Panels), there needs to be clear guidance on how to easily decide which one takes priority.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I would think most of those modules would do their title changing late enough that it wouldn't matter. If title changing is done in _preprocess_page(), the view will already be done.

In any case, it's a change in behavior. With half a million sites, even if it affects only .01% of them, it could still negatively impact thousands of sites. And I'm willing to do this when it's especially important, but I'm not yet convinced this is the right change.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

The patch in #9 would not introduce any regressions, unless someone specifically set the view title to '<none>' which is a little unlikely.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

That is incorrect. If a view has no title, and some other module was doing a drupal_set_title() in the background that the view was overriding, this change would cause that title to now become the page title, because the View would no longer try to enforce its idea that the empty title is correct.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Title: Don't set the page title if it's empty » Allow the title to be not assigned if has the value "<none>", matching core block functionality

@merlinofchaos: Patch #9 is completely different to the original patch, please explain how #9 would make any difference to existing sites unless the title was specifically set to '<none>'.

Updated the issue title to be more accurate.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I'm pretty sure I said in #15.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

@merlinofchaos: .. but only if the title value was specifically set to '<none>', which is quite unlikely:

-    drupal_set_title(filter_xss_admin($this->view->get_title()), PASS_THROUGH);
+    $title = $this->view->get_title();
+    // Support the core method of using '<none>' to indicate nothing should be
+    // assigned to the title, so only process the title value if it is not that
+    // value.
+    if ($title != '<none>') {
+      drupal_set_title(filter_xss_admin($title), PASS_THROUGH);
+    }

Again, the only change would be if someone just happened to set the title to '<none>', if someone left the title blank then it would make no difference with how Views works today.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

No, that's in the new code.

In the old code, that you're replacing, if the title was set to '' then it would be set to ''. In your code, the title will be left alone.

I don't see how this *isn't* a change, since that's your whole purpose.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Category: bug » feature

I've updated the issue description to match the new proposed functionality, and changed it to a feature request to indicate that I'm no longer considering it a bug (though I do still consider it an undocumented feature :p ). Please disregard patch #2, patch #15 is the only one that's relevant.

So, because I'm not quite following it, please explain how patch #15, which includes the lines quoted above in #18, would change functionality for any existing site unless they were specifically filling in "<none>".

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

Merlin, in #10, you said

"... shouldn't it just actually tell the view what the title should be?"

Why didn't I think of this? It makes perfect sense in hindsight, but it didn't even occur to me that since the view was altering the title I wanted, I should just tell the view to use the title I wanted. I've changed my code to do just that.

Perhaps an alterative to this patch would simply be some kind of documentation (maybe even a comment accompanying the line of code in question here) that explains to developers that they should change the view's title, rather than setting the title themselves and having the view override it.

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented
Issue summary: View changes

Updated the issue description to indicate the new direction for this new feature.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Title: Allow the title to be not assigned if has the value "<none>", matching core block functionality » Allow the page title to be not assigned if it has the value "<none>"
Issue summary: View changes
Valentine94’s picture

Valentine94
Location Kyiv, Ukraine
CreditAttribution: Valentine94 at Drupalway commented

Looks good to me, +1 to RTBC

  • DamienMcKenna committed b31303e on 7.x-3.x 
    Issue #1760494 by DamienMcKenna: Allow the page title to be not assigned...
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs review » Fixed
Parent issue: » #2855120: Plan for Views 7.x-3.16 release

Committed. Thanks everyone.

Status: Fixed » Closed (fixed)

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