In simplenews.source.inc line 632 we have:

unset($build['#theme']);

Just wondering what the purpose for this is and whether there is another way to achieve it as it prevents you being able to set up your own theme functions for it. In our case, it stops panelizer from being able to take over the rendering, which is a shame as Panelizer is a nifty way to compose/format an email!

CommentFileSizeAuthor
#19 simplenews-remove-unset-node-theme-7.x-1.x-1914138-19.patch594 bytesjsacksick
#18 simplenews-unset_default_theme_node_only-1914138-18.patch876 bytesosopolar
#11 1914138-11-only_unset_default_theme_any_entity.patch1.02 KBandrewbelcher
#9 1914138-9-only_unset_default_theme_any_entity.patch844 bytesandrewbelcher
#5 1914138-only_unset_default_theme_any_entity.patch812 bytesandrewbelcher
#5 1914138-only_unset_default_theme_node_only.patch796 bytesandrewbelcher

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The reason is that we do not want the default node template markup just like we don't want the default field markup.

A possible solution could be to only remove it only if has the default values and to assume that "you know what you're doing" if not.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

That sounds like a great idea.

andrewbelcher’s picture

andrewbelcher commented

A possible solution could be to only remove it only if has the default values and to assume that "you know what you're doing" if not.

I suppose that comes with the risk of other modules doing something iffy. Would having some kind of hook for modules to register themselves with if they properly handle things be sensible? I suppose there is a balance of avoiding conflicts and allowing the use of other modules...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

There isn't really a reason to replace the #theme property of e.g. a node unless you are panelizer or display suite I think. So IMHO, we can safely assume that if this happens, it happens for a reason.

field #theme functions might be a bit different.

However, it is IMHO fine to go with this for 7.x-2.x and then check if users report any bugs/issues about it. Using a hook or something would require that those modules explicitly integrate with simplenews and that's always a bit tricky (where does that code live and so on).

andrewbelcher’s picture

andrewbelcher commented
Status: Active » Needs review
StatusFileSize
new 1914138-only_unset_default_theme_node_only.patch796 bytes
new 1914138-only_unset_default_theme_any_entity.patch812 bytes

Ok, so here is a patch that should solve it. I suggest we want the 2nd one unless I'm wrong about my assumption that the default entity theme uses the entity type...

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented

The 'any-entity' stuff is progressing quite quickly, so I would definitely go with the second patch.

+++ b/includes/simplenews.source.incundefined
@@ -643,7 +643,9 @@ class SimplenewsSourceEntity implements SimplenewsSourceEntityInterface {
+    if ($build['#theme'] == $this->getEntityType()) {

Does this need to include a check for whether the #theme is 'entity' or is the use of entity.tpl.php controlled by the template suggestions?

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Status: Needs review » Needs work
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, I think it should check for both 'entity' and getEntityType(). entity types that rely on entity.module usually use #theme entity and then work with template suggestions (entity__$entity_type) if they need a custom template but #theme should always be entity.

andrewbelcher’s picture

andrewbelcher commented
Status: Needs work » Needs review
StatusFileSize
new 1914138-9-only_unset_default_theme_any_entity.patch844 bytes

Ok, that's updated!

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Status: Needs review » Needs work 
+++ b/includes/simplenews.source.incundefined
@@ -643,7 +643,9 @@ class SimplenewsSourceEntity implements SimplenewsSourceEntityInterface {
+    if ($build['#theme'] == 'entity' || $build['#theme'] == $this->getEntityType()) {

I feel like this should have a comment explaining the thought process of only unsetting it if it's the default.

andrewbelcher’s picture

andrewbelcher commented
Status: Needs work » Needs review
StatusFileSize
new 1914138-11-only_unset_default_theme_any_entity.patch1.02 KB

Ok, here's with some documentation!

    // We need to prevent the standard theming hooks, but we do want to allow
    // modules such as panelizer that override it, so only clear the standard
    // entity hook and entity type hooks.
rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented

Looks good to me!

berdir’s picture

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

Agreed, commited and pushed.

Display Suite might need a different check as that AFAIK keeps #theme node. But we can deal with that separately.

Status: Fixed » Closed (fixed)

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

Status: Closed (fixed) » Needs review

Olivier Beerens queued 11: 1914138-11-only_unset_default_theme_any_entity.patch for re-testing.

Olivier Beerens’s picture

Olivier Beerens commented
Issue summary: View changes

Doesn't work with simplenews 7.x-1.1

Status: Needs review » Needs work

The last submitted patch, 11: 1914138-11-only_unset_default_theme_any_entity.patch, failed testing.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented
Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new simplenews-unset_default_theme_node_only-1914138-18.patch876 bytes

Backport to version 7.x-1.x-dev.

jsacksick’s picture

jsacksick commented
StatusFileSize
new simplenews-remove-unset-node-theme-7.x-1.x-1914138-19.patch594 bytes

I'm also wondering why we need to get rid of the default node template?
I'm customizing the node template (Putting html around fields, embedding views via a preprocess_node function) for both the email version and the node full page.
I'm attaching a patch that removes the unset line for the 1.x version (I guess this won't be committed but I need it on d.o for my make file, and this could be useful for other users).

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented

@jsacksick: Are using Panelizer? I guess the unsetting is necessary for those not using Panelizer but want to set a theme different to the default theme. Did the patch in #18 not work for you? As it is the backport it would be more likely to get it in and even more if it gets reviewed.

candelas’s picture

candelas commented

Thanks @osopolar , patch in #19 works.

Please @Berdir add this to 7.x-1.x-dev :)
Since you are very busy with D8 I don't think you will have time for 7.x-2.x-dev and it will make it easy for future port to D8.
By the way, a good opportunity to say thanks for all your generous code sharing in Drupal ;)