Re-introduce per-content type visibility to block settings [#453254]

Drupal 4.6 used to have per-content type visibility for blocks, but that was removed in Drupal 4.7 (Some people had gripes about that: http://drupal.org/node/67499, http://drupal.org/node/48569). However, for some settings cleanup around content type help messages (which are ought to become per-content type limited visible blocks for node editing), I think we should reintroduce this feature. Also, many people request this feature and it is blogged as a PHP snippet tip around many places on the internet: http://www.google.com/search?q=drupal+content+type+specific+visibility

I think the per-content type visibility settings were removed in the name of making the interface easier, but I don't consider instructing users to copy-paste PHP code from possibly untrusted sources and running their sites with them a better solution.

The attached patch loads up all blocks which have no content type limitations and only loads up blocks which were set to show up on specific node types when a node of the specific node types are shown, edited or any other subtab is shown under the node (path based limitation can already limit this further in Drupal).

The code works for me on testing with a simple article node and setting up a block limited to the article content type. It does show up when seeing the node or editing it, but it does not show up when seeing other nodes, or not seeing a node. An upgrade path would obviously be required, which would create this new table. Since no such feature existed for quite some time now, there would be no data to migrate. I'll add in the update function is this feature is validated to be worthwhile and accepted. Also, a CHANGELOG entry would be useful IMHO.

Marking migrating help texts for content types at #448784: Migrate content type help to custom blocks postponed on this one.

Comment	File	Size	Author
#32	453254-update.patch	3.36 KB	agentrickard
#32
#29	453254-29-upgrade.patch	1.49 KB	Pasqualle
#29
#22	453254-22-block_visibility_node_type.patch	7.94 KB	Pasqualle
#22
#16	453254-16-block_visibility_node_type.patch	7.94 KB	Pasqualle
#16
#14	453254-14-block_visibility_node_type.patch	6.99 KB	Pasqualle
#14
#11	453254-11_block_visibility_node_type.patch	6.66 KB	Pasqualle
#11
#2	Content-filter.jpg	21.9 KB	Gábor Hojtsy
	block-visibility-type.patch	5.35 KB	Gábor Hojtsy

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Damien Tournoud CreditAttribution: Damien Tournoud commented 4 May 2009 at 15:57

I would rather introduce some base context-like features into core. We already have per path and per role visibility settings, this patch suggest to introduce per content type, what's next? Per user id? Per day of the week? Per season? etc.

Putting everything into the block edit interface will simply not scale.

Comment #2

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 4 May 2009 at 16:06

File	Size
Content-filter.jpg	21.9 KB

I agree it is already confusing and with this patch it becomes more confusing. I believe however that we should be able to put on more user friendly interfaces on top of what we have now. Nothing tells people currently that all conditions should be met at the same time for example.

I would suggest that blocks would have a rule UI, where you can cook up the rules on when they show up much like you build up a filter for the content type admin listing. You add up and remove items and then see all the filtering criteria at once. I think it helps you understand the and/or relationships. Well, that would still need to support removing rules from the middle of the list, etc, but you get it.

Anyway, I agree that blocks visibility configuration is messy, and the existing options are already confusing (I'd guess that content type visibility was removed because it was confusing). Reintroducing this feature however is the only way I could fix #448784: Migrate content type help to custom blocks, so if we are not going to do this, then I need to mark that won't fix and live with custom setting of help texts that way. I am certainly not up to work on revolutionizing block visibility given how full my plate is already. :)

Comment #3

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 4 May 2009 at 16:55

Comment #4

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 4 May 2009 at 17:32

Also, we'd obviously need tests for this if the new feature is approved.

Comment #5

catch

he/him

English

CreditAttribution: catch commented 4 May 2009 at 18:32

Note there's #370172: refactor block visibility which tries to take the visibility checks out of block module's critical path (even they end up being block's implementations of its own hooks). No interface help though.

I don't think the fact that our existing block interface doesn't scale well is a good reason to avoid putting new features in - same as I don't think the user permissions page is a reason not to make our permissions more granular. However having done a site with lots of regions and blocks and no panels for the first time for a while I remembered how much of a pain it is to deal with large numbers of blocks - so we do need to fix that, I just don't think this patch necessarily has to be dependent on it.

Comment #6

akahn CreditAttribution: akahn commented 5 May 2009 at 18:43

Hi, Gábor. I'm getting the following error on my site after applying this patch. Am I doing something wrong?

PDOException: SELECT DISTINCT b.* FROM {block} b LEFT JOIN {block_role} r ON b.module = r.module AND b.delta = r.delta LEFT JOIN {block_content_type} c ON b.module = c.module AND b.delta = c.delta WHERE b.theme = ? AND b.status = 1 AND (r.rid IN (?) OR r.rid IS NULL) AND ( c.type IS NULL) ORDER BY b.region, b.weight, b.module - Array ( [0] => garland [1] => 2 ) SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.block_content_type' doesn't exist in _block_load_blocks() (line 581 of /Users/akahn/drupal-7/modules/block/block.module).

As someone who builds Drupal sites, this feature would be great to have. I think that just about any reasonably complex web site build with Drupal needs this feature, and I always do it with PHP, which is ok, but it leaves the editors/managers of the site out of the process of block visibility settings. More work for me. ;) A more context-like solution would be better, as Damien suggests, but in the mean time this will help.

Comment #7

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 5 May 2009 at 19:59

@akahn: block_content_type is a new table, you need to reinstall Drupal.

Comment #8

catch

he/him

English

CreditAttribution: catch commented 5 May 2009 at 21:08

gabor - could you run an explain on the modified query and post before/after results?

Comment #9

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 6 May 2009 at 20:45

Patch reviewed and works fine.

Comment #10

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 7 May 2009 at 07:22

Status:

Needs review

» Needs work

Turns out that to reproduce the functionality in #448784: Migrate content type help to custom blocks, we also need to match on paths when the node is created (node/add/$nodetype), when the admin selects the given type. The admin can still exclude such paths via the path admin interface. But the admin cannot add up that path type, since all conditions need to be met at the same time, and node/add/$nodetype does not have a menu_get_object() of type $node.

Comment #11

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 17 May 2009 at 00:18

File	Size
453254-11_block_visibility_node_type.patch	6.66 KB

this patch is an incremental patch after the patch in #370172-30: refactor block visibility

1. fixed the problem in #10
2. fixed a missing db insert at block creation
3. renamed the table to block_node_type

note: please do not change the issue status to CNR as the testing bot can't apply this incremental patch.. You may change it to postponed (on #370172) if you agree that this is the right way to solve this feature..

Comment #12

catch

he/him

English

CreditAttribution: catch commented 17 May 2009 at 00:25

One minor thing:

+  foreach($result as $type) {
+    $default_type_options[] = $type->type;
+  }

Can use ->fetchCol() here I think.

Comment #13

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 8 June 2009 at 11:37

Now that #370172: refactor block visibility is committed, this patch can now build on Drupal 7 HEAD as it is, without depending on other patches.

Comment #14

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 8 June 2009 at 18:36

Status:

Needs work

» Needs review

File	Size
453254-14-block_visibility_node_type.patch	6.99 KB

reroll

Comment #15

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 9 June 2009 at 06:30

Patch looks good on a visual inspection. The only concern I have is that we usually don't do these constructs:

if (something) {
 // do nothing, just a comment
} else {
  // do the stuff here
}

We instead usually do

if (!something) {
  // do the stuff here
}

Comment #16

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 9 June 2009 at 14:44

File	Size
453254-16-block_visibility_node_type.patch	7.94 KB

fixed #15

Comment #17

13 June 2009 at 12:51

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #18

lilou CreditAttribution: lilou commented 13 June 2009 at 13:39

Status:

Needs work

» Needs review

Setting to 'needs review' - testbot was broken.

Comment #19

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 22 June 2009 at 15:31

Marked #496334: GUI for block visibility per content type as duplicate.

Comment #20

Dries CreditAttribution: Dries commented 22 June 2009 at 20:01

Status:

Needs review

» Reviewed & tested by the community

This looks ready to be committed. Marking as such.

Comment #21

24 June 2009 at 01:20

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch failed testing.

Comment #22

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 24 June 2009 at 19:16

Status:

Needs work

» Reviewed & tested by the community

File	Size
453254-22-block_visibility_node_type.patch	7.94 KB

reroll. no code changed, same as #16.

Comment #23

27 June 2009 at 17:45

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch failed testing.

Comment #24

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 30 June 2009 at 22:13

Status:

Needs work

» Reviewed & tested by the community

patch #22 still applies cleanly..

Comment #25

Dries CreditAttribution: Dries commented 1 July 2009 at 08:04

Status:

Reviewed & tested by the community

» Fixed

Committed to CVS HEAD. Thanks.

Comment #26

catch

he/him

English

CreditAttribution: catch commented 1 July 2009 at 08:53

Category:	feature	» bug
Priority:	Normal	» Critical
Status:	Fixed	» Needs work

No upgrade path from D6.

Comment #27

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 1 July 2009 at 09:13

@catch: what kind of upgrade do you expect for this? It is a new setting, which does not require a default value..

Comment #28

catch

he/him

English

CreditAttribution: catch commented 1 July 2009 at 09:20

Umm, the new database table that was introduced and is queried on every page load?

Comment #29

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 1 July 2009 at 10:03

Status:

Needs work

» Needs review

File	Size
453254-29-upgrade.patch	1.49 KB

Comment #30

catch

he/him

English

CreditAttribution: catch commented 1 July 2009 at 11:13

Status:

Needs review

» Reviewed & tested by the community

That's the one.

Comment #31

Dries CreditAttribution: Dries commented 1 July 2009 at 12:03

Status:

Reviewed & tested by the community

» Fixed

Committed to CVS HEAD.

Should this live in block.install instead?

Comment #32

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 3 July 2009 at 19:08

Status:

Fixed

» Reviewed & tested by the community

File	Size
453254-update.patch	3.36 KB

Yes. It should.

Comment #33

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 4 July 2009 at 04:23

Status:

Reviewed & tested by the community

» Fixed

Third time's a charm? ;) Committed to HEAD.

Comment #34

HedgeMage CreditAttribution: HedgeMage commented 9 July 2009 at 17:07

Status:

Needs work

» Reviewed & tested by the community

OOPs! Did that in the wrong tab.

Comment #35

9 July 2009 at 17:05

Status:

Fixed

» Needs work

The last submitted patch failed testing.

Comment #36

HedgeMage CreditAttribution: HedgeMage commented 9 July 2009 at 17:08

Status:

Reviewed & tested by the community

» Fixed

Oops... I commented in the wrong tab... didn't mean to do anything to this issue...returning to previous status of "fixed"

Comment #37

YesCT CreditAttribution: YesCT commented 9 July 2009 at 17:13

I think it was an oops to move from fixed to rtbc. Moving back.

Comment #38

23 July 2009 at 17:20

Status:

Fixed

» Closed (fixed)

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

Comment #39

bgogoi CreditAttribution: bgogoi commented 24 December 2015 at 10:15

hello! anyoen tested this patch? I need this urgently https://www.drupal.org/node/2639786