Bean update 7.x-1.13 Causes Duplicate Headers

Does anyone else have this issue?

@fujishooter are you overriding the block or entity html output at all?

I faced the same issue. It was caused by https://www.drupal.org/node/2084823. For contextual links we should have displayed title prefix and suffix, but we should not have rendered the title itself.

Looks like the patch in #3 passed tests after a retest.

Does anyone else have this issue?

@indytechcook: Yes. We use the Bean module in a custom Drupal distribution used on hundreds of sites used within our organization. We recently updated the version of Bean packaged with our distribution to 7.x-1.13. We started seeing this issue as soon as we started rolling the update out to sites and, for sites that are using a lot of beans, it's kind of a big problem because bean blocks that have been in use for a long time are suddenly showing duplicate / double titles.

FWIW, our initial efforts to remedy the issue were compounded by a couple of somewhat related issues.

We noticed this comment in #2084823-9: contextual links for entity view:

Be aware that titles will be displayed by default with this patch. If this is not desired behavior, I suggest people to override the template and/or install the module title.

Because we also package the contrib Title module with our distribution, we tried to hide the titles using that module (as suggested) but we encountered the problems described in #2701835: Add support for empty title when using Title module. Additionally, when we considered the other suggestion of adding custom templates of our own to override bean.tpl.php we were concerned that doing so would exacerbate the problems we've seen with titles not showing up when we expect them to, described in #1858416: Rendered bean does not display the title when display settings indicate it should.

We've tested patch #3 on a number of these sites and it fixes the problem for us. We're going to go ahead and add this patch to our custom distribution, at least for now.

I agree with @Alex Bukach that the title probably should not be rendered in the template, even if the title suffix & prefix are needed for other reasons.

I agree with @Alex Bukach and @joegraduate about the inclusion of the title in the template. One other issue that may or may not be related, but the bolck configure and core block edit contextual links are now missing from the contextual links menus.

Same issue here after updating Bean. If for some reason this rendering of the title were intentional, there should at least be a class on the title so it can be easily targeted with styles. I had considered addressing this problem by removing my panel and block titles and using the new h2s provided by Bean, but was surprised to see that there was no class on the h2.

We are in a similar situation than #6 we tested this patch and will included on the module as the it fixed the issue.

After some more testing on this patch,we found problems because it changes the structure of the html.

#3:

For contextual links we should have displayed title prefix and suffix, but we should not have rendered the title itself.

We should not render the title, really? Why not? Does it make any sense to have only a prefix and a suffix? Maybe in some particular cases, but as a generic rule it seems odd.

#6:

we tried to hide the titles using that module (...) but we encountered the problems described in #2701835: Add support for empty title when using Title module.

That issue should be fixed and it has a patch ready to be tested. It shouldn't impact us here.

#6:

we were concerned that doing so would exacerbate the problems we've seen with titles not showing up when we expect them to, described in #1858416: Rendered bean does not display the title when display settings indicate it should.

That issue should be fixed and it has patches and a discussion going on (by the way, It says that using Title module should avoid the problem and @joegraduate this is not aligned with your testimonial here). Participate, test, give feedback there please. It shouldn't impact us here.

#9

there should at least be a class on the title so it can be easily targeted with styles

There is place for css classes in the $title_attributes variable. Anyway, @BrightBold, you can submit a patch to improve this if you think it should be improved.

#11

After some more testing on this patch, we found problems because it changes the structure of the html.

Then I assume using this patch will break your site, so it shouldn't be commited. It would be interesting to have a more consistent feedback about your issue and that you move this issue into another state because you leaved it still in RTBC like if it's ok for you.

In my opinion we should have a consistent HTML structure. Having prefix and a suffix with nothing in the middle in not logical.
If we go now back in this, there will be people complaining because in the meanwhile they are using the title and they'll be unhappy if we remove it now.
So, let's just stick into what is correct. I understand the pain of fixing some consequences, but still there are no impediments to do it.

Sorry, but according to #11 and my own comment, I'll change the status to Active and keep this open for more discussion.

I'd just like to call out that although this change is aligning with best practices and gets this issue to where it should have been in the first place, this seemingly small change is actually a large problem for many users of the previous versions.

With Drupal 7 fading and Drupal 8 taking off I imagine you will be affecting more existing websites than new ones. Changes like this should come with upgrade paths or be separated out into an appropriate release name. A minor point release that requires the existing users to take action on their own accord to be able to use the module does not seem like a best practice. There are a number of release types that could have been used to solicit feedback or issues prior to a minor point release. This feels like it should have been an alpha or beta release first and an upgrade path should have been considered. This is how I understand the release naming practices: https://www.drupal.org/node/467020

If we go now back in this, there will be people complaining because in the meanwhile they are using the title and they'll be unhappy if we remove it now.

This statement could have been avoided with a more careful release and a similar statement could be said for those that are already using this module. "As a user of this module for many years, this change removed a feature that I relied on and I am unhappy that you did at my expense for new users."

So that leaves me to this question, can this be amended so that an upgrade path is available? Can we make the title optional through the UI somehow and default those on < 1.13 to not visible?

@sherakama you're right, this should have been done in a new release, not in minor upgrade.

What to do? For me it's a possibility to go back and put the change in a new release.

But isn't it more easy for you to deal with it now?
Anyway, you'll need to tackle this someday, even if it comes in a new release, no?
Can we try to find an easy alternative to everyone before juggling with the releases?

My suggestions:
Just write a preprocess hook to set the "$bean->title" to empty.
Just write a new bean.tpl.php in your theme to override the default one.

Wouldn't this be an easy straightforward solution for you?

To give you an idea of the scope of this issue for me here goes:

I help maintain a platform of roughly 2500 websites running on roughly 7 different codebases that allow for custom themes. Some of those themes are on a base theme, some of them are 100% custom, and some are in the middle. Going through each of the sites and updating the bean.tpl.php file is not an efficient option for me. So that narrows it down to the 7 different codebases and the common modules/profiles between them.

Approaches:

1. Create a preprocess hook to unset the $block['subject'] for bean blocks and allow the new template variable to take lead.
This sort of works but has a couple of problems. It conflicts with those sites where the maintainer has used block titles and bean titles for different purposes. For those that only used the default bean-to-block title $block['subject'], by moving the title to the BEAN template I mess with their css selectors.

2. Unset the $bean->title in a preprocess hook.
For those that use the BEAN title in their output already through another contrib means unsetting the title in a preprocess or alter hook may un-do their use case.

3. Leaving preprocesses alone and running an update hook to set all block configurations where the title is empty to <none> works for those blocks that already exist as the $bean->title gets rendered but the $block['subject'] does not but has two problems. The first problem is the css issue again where the markup shifts and css selectors break. The second issue is a usability issue where when a user creates a new bean and places it in a block region both titles show up until they also edit the block configuration and set it to none.

So right now, I am trying to find a way to use the $block['subject'] output for the title when the block configuration for the title is empty and the $bean->title is set but only if the $bean->title is not being used in the display as another element.

Perhaps I am over thinking this? I am all ears to alternate solutions.

Thanks for listening.

@sherakama I could feel a glimpse of your pain.
Thanks for sharing and I hope some of your suggestions can help others.

I won't block any solution here. After this discussion, I myself put back the ticket into RTBC (like it was before I changed).
I think it 's worth to consider to apply this patch here and then to revert it in a major release.

@sherakama, my absolute gross hack until there is a long term fix has been to put in a regexp in a THEME_preprocess_block function.

function THEME_preprocess_block(&$variables) {
  if(isset($variables['elements']['bean'])) {
    // scrub the first H2 from the content string
    $variables['content'] = preg_replace('/<h2>(?:(?!\/h2).)+\/h2>/i', '', $variables['content'], 1);
  }
}

This may not match your use case, but in ours this is the fastest path to a fix that is easily reversible. When this issue is better resolved it's a matter of removing a single LOC. We have custom templates for each bean type, distributed across a bunch of codebases, so we're not in the position to be able to tweak templates either.

Thanks for the feedback and suggestions.

I think we are going to go with removing the title in hook_preprocess_entity as from a quick round of testing, seems to be the path of least resistance. For those that did use the title in a meaningful way, they usually found a contrib or new field solution so doing this doesn't seem to have too many adverse effects on our stacks.

/**
 * Implements hook_preprocess_entity.
 */
function MYMODULE_preprocess_entity(&$variables) {
  // Only run on beans.
  if ($variables['entity_type'] !== 'bean') {
    return;
  }

  // No bean template title.
  $variables['bean']->title = '';
}