Ensure block delta is class safe

I cannot verify this issue on Drupal 6.0 final, using PostgreSQL 8.2.3, though.

Can you still reproduce this error? If so, what is the exact error message from the database?

Nice report. Bumping status.

OK, so as I understand the issue, the problem is this.

In block_box_get, we run the following query:

SELECT bx.*, bl.title FROM boxes bx INNER JOIN blocks bl ON bx.bid = bl.delta WHERE bl.module = 'block' AND bx.bid = 1

And since bx.bid is an integer field and bl.delta is varchar, then PostgreSQL 8.3 will not process the query.

Let me try some patch code and see that it doesn't break pgsql 8.2.3, and then I'll give you something to test.

Do you have a link to that pgsql notice page?

Note: since this only affects the 'configure' and 'delete' operations, I am not marking this as critical, but it still may be.

Do you have the ability to run EXPLAIN on this query, and report the results?

SELECT bx.*, bl.title FROM boxes bx INNER JOIN blocks bl ON bx.bid = bl.delta WHERE bl.module = 'block' AND bx.bid = 1

Bumping to critical, as this needs some schema eyeballs on it.

Try this replacement code in block.module:

function block_box_get($bid) {
  return db_fetch_array(db_query("SELECT bx.*, bl.title FROM {boxes} bx INNER JOIN {blocks} bl ON CAST (bx.bid AS text) = CAST (bl.delta AS text)  WHERE bl.module = 'block' AND bx.bid = %d", $bid));
}

Or just this query in pgSQL:

SELECT bx.*, bl.title FROM boxes bx INNER JOIN blocks bl ON CAST (bx.bid AS text) = CAST (bl.delta AS text) WHERE bl.module = 'block' AND bx.bid = 1

THe above will not work on MySQL without editing. See http://dev.mysql.com/doc/refman/5.0/en/cast-functions.html

The following should work on both platforms:

SELECT bx . * , bl.title
FROM boxes bx
INNER JOIN blocks bl ON CAST( bx.bid AS char( 32 ) ) = CAST( bl.delta AS char( 32 ) )
WHERE bl.module = 'block'
AND bx.bid =1

CHAR is set to 32 because that is the max allowed value of bl.delta.

Proper patch. Marked as critical because this error may bite us any time we do a JOIN on different data types.

I count ~180 JOIN statements in Drupal core. Most will not be affected. Will try to compile a report.

OK, I searched through the core code for JOIN instances and looked at all of them. Most are of the format:

    $result = db_query('SELECT r.rid, p.perm FROM {role} r LEFT JOIN {permission} p ON r.rid = p.rid ORDER BY name');

Since rid == rid, I am assuming that data type conversion is not an issue.

In some cases, there are slightly different column names such as:

        FROM {menu_links} ml INNER JOIN {menu_router} m ON m.path = ml.router_path

In those cases, I verified that the columns are the same data type.

Result: It appears we only have this one instance to fix (see patch in #10).

@drupalifm

We have an ongoing need for pgSQL testers -- as you can tell, I only use 8.2.x, so I never saw this. In answer to:

I was wonder if this problem would occour in all JOINS or other SQL statements, but it seems not.

Yes, it does, that is why I changed the issue title. However, I examined all the JOIN statements in Drupal core and it seems that you found the only error.

See #12 for the findings.

If the patch works for you, you should mark it "patch (ready to be committed)" -- by policy, I cannot set that status on my own patch.

NOTE: This bug affects Drupal <6 also. See http://api.drupal.org/api/function/block_box_get/6

So the patch will need a backport.

For future reference, the reason this only appears in one place is that the Block system is the only section of the code that allows mixed keys.

$block['delta'] can be an integer or a string (hence the varchar datatype) for module-defined blocks. The boxes->bid is an integer only value, assigned to user-created blocks.

The proper fix for D7 is to eliminate the use of varchar in $block['delta'] -- or to make boxes->bid a varchar as well.

This will have to be fixed in HEAD first, and then in DRUPAL-6, and then in DRUPAL-5...

Patch should apply to HEAD. Note: query tested correctly on MySQL 4.1.x, pgSQL 8.2.x and pgSQL 8.3.x.

Subscribe.

subscribe

I thought we are going with ANSI SQL.

In which case the schema chance described in the last line of #18 is the proper fix, yes?

Could somebody explain what the status is of this bug and its possible fix?

Thx

Gijs

The patch in #10 will solve the issue, but is probably not a permanent solution. The permanent solution is to change the underlying data type in the SQL schema.

Ran into this issue running drupal 5.7

Applied patch - works great

Instead of having to introduce a cast, I'd prefer to align the data types. It's cleaner.

Just to agree with #27. I get the same problem with Drupal 5.7 running postgresql 8.3.0

@Dries

Yes. The cast is a temporary patch for users of 5.x and 6.x. For 7.x, we should either make both columns varchars or remove the use of string deltas in hook_block.

Making both varchar is easier. Making both integers is cleaner.

Block deltas are no longer numeric; all core deltas are now strings in D7. Thus making the deltas an integer in the database would be a feature regression.

So, if deltas are strings, then boxes -> bid should be a varchar. Right?

@agentrickard: Yes.

*pauses to actually look at the code involved for a moment*

Deltas have always supported strings ({blocks} table), but most core modules did not, in fact, use strings for them. The block module in particular uses an integer because it defines an arbitrary number of blocks (stored in the {boxes} table), per admin configuration. Because that data is stored in a table, a surrogate key PKey (bid) makes sense, and is naturally an int. However, that then is also used as the delta, which results in a join of an int (boxes) against a varchar (blocks), and hilarity ensues.

agentrickard is correct that the fix is to use varchar for both fields, since we cannot use int for both fields. However, that then means that block module cannot rely on a serial field for bid, which must be a unique value of some sort. So the options I see are:

1) Find some other way of maintaining {boxes}.bid as a unique (possibly numeric) value in a varchar field. At not quite midnight I cannot think of a nice way of doing that, especially now that db_next_id() is gone.

2) Drop the join and issue two queries; one to load the data from {boxes}, and then another to load the corresponding data from {blocks}. We can then rely on the DB layer doing the necessary type casting.

I am not sure which method is better. Probably #1 if we can find a good alternative solution for maintaining a unique PKey, and #2 if we cannot (for some Dries-defined definition of "good").

Note that this is an issue only for the block module because it is the only module that tries to join the block table against anything in order to get its data. Virtually every other module simply references a callback function or block of code that does whatever it's going to do.

Moving back to the most recent affected version.

You really shouldn't be joining on different types like that. It shows incorrect design at the database level. We should probably find a way to specifically fix the code for blocks to be a little more thoughtful about how it gets it's data. It sounds to me like what really needs to happen is a surrogate join that provides the natural data on result but the key is always via the integer key.

There's no need for a surrogate key on the blocks table. All that would do is make the maintenance of it that much more difficult. The bug here is in the block module's block_block() implementation joining across data types. That should be fixed so that it is not joining across data types.

@Crell

Is there any good reason why all block deltas are now strings? It would be simpler to force those to be ints, wouldn't it?

Otherwise, the "solution" may be to store a bid string in the variables table, or to generate a random hash string (maybe 6 digits, a-z|0-9) for each bid -- and test for its existence before insert.

The original patch is here: http://drupal.org/node/216072

The original reason was to make theming easier, since the delta is used as part of a class name. It also makes the code and block definitions more predictable. I also want to convert hook_block to be more like hook_menu (sometime after the database work is finished), for greater flexibility and performance. That will require a string delta to work properly. Basically, if a human is ever going to see it then a string is far nicer to work with than an int, and humans do see deltas.

Another option, though, is to simply have block.module have a delta of "block_$bid", a la aggregator. I suppose then the bid would have to be uniquely managed via the variables table, as agentrickard suggests. That's a possible race condition, but the odds of 2 admins creating custom blocks within a second of each other is sufficiently remote that I don't think we need to worry.

I would support 'block_$bid' as the best option -- assuming that removing strings from the delta is a non-starter.

(a strongly typed database is a very poor match to a loosely typed script language)

@chx well, be that as it may. See http://drupal.org/node/220064#comment-729199 for the (rather annoying) pgSQL stance on the issue.

But if we want database portability....

Could we have a unique machine readable name for the boxes table and have that as primary key? Then I get #block-myblockname for CSS and .tpl.php files which would be worth having anyway.

@catch

There are two obvious ways to accomplish that:

-- A set array of strings to use (red, white, blue, etc.)
-- A random hash string (4-6 characters).

Problem with approach #1 is that we have to set aside 100, 500, or 1000 (?) strings. (Really an unknown number).

Problem with approach #2 is that random strings aren't human-readable.

And don't forget that we have to provide an upgrade path for current users.

block_$bid gives us a human-readable and machine-readable unique value, with only a very tiny potential for race condition (see above). That works for me.

The alternative is to use ints in {boxes} and block_$bid in {blocks}, and do two queries. That may require some limited string parsing to get the $bid back out of $delta (just a substr() call, really), but it would give us confirmed-unique ints in {boxes} while keeping string deltas at the conceptual level. It also would mean no schema change (I think), which is nice.

I do not believe removing string deltas is a good move at all; they've been a feature for years, just one core didn't make use of. Many contribs do. This problem is caused by the joining of a string and int field, which was always a bug; it just didn't show up until now.

afaik boxes aren't created by modules. So at the moment, only an adimistrator creates them. Same as content types we can require in validation that this be unique. Additionally, existing deltas are already a unique integer, so converting them into a unique string is no big deal for an upgrade path. Am I missing something?

/me goes to look at the block creation page.

Well, since block description is required, we could always turn that into a lowercase alphanumeric string and use it for the delta.

How about that for string deltas? So, for example, I create a block called "custom login", the delta will be custom-login.

And since the 'info' column in {boxes} already has a UNIQUE constraint, we don't have to worry about duplicate strings.

[Exceprted from block_schema()]
        ...
        'info' => array(
        'type' => 'varchar',
        'length' => 128,
        'not null' => TRUE,
        'default' => '',
        'description' => t('Block description.'),
      ),
    'unique keys' => array(
      'info' => array('info'),
    ),
    ...

For the upgrade, we simply change BID to varchar and run a script to convert INFO strings to the new BID value.

Sound like a plan? If so, I'll try to code something to make this work.

Hm. That sounds like a viable direction. We probably should still check for uniqueness in PHP-space for robustness, but since everything else is going string deltas it makes sense to do the same here. The upgrade path should be reasonably straightforward, too.

However, I don't know that we ca do that for D6, since that would be a change in the classes that get generated for theming. (A schema change is impossible to avoid here, since it's the schema that is the bug.) +1 for that approach for D7, undecided for D6/5.

Right. For 5/6, I recommend the patch in #10.

OK. I don't have access to CVS right now, so apologies for three patches instead of one.

The following patches:

- Change boxes.bid to a varchar(128)
- Change block.delta to varchar(128) for consistency.
- Convert the block.bid to a string, lowercase, using preg_replace to turn non-alphanumeric characters into '-'.
- Provides an update function for system.install

The preg_replace logic probably needs a little work. It was cribbed from bootstrap.inc.

OK. Proper patch attached, against CVS HEAD.

Two issues here:

-- The $delta / boxes.bid is now taken as a strtolower() of the block description. This means that you cannot create a block named "test" and a block named "Test." This may not be acceptable to some users.

-- I am uncertain how the preg_replace logic will affect languages other than English.

Fixes a comment typo.

Fixes missing %d to %s conversion.

subscribe

Note on non-English strings.

Some research digs up: http://us.php.net/manual/en/reference.pcre.pattern.modifiers.php

 u (PCRE_UTF8)
    This modifier turns on additional functionality of PCRE that is incompatible with Perl. Pattern strings are treated as UTF-8. This modifier is available from PHP 4.1.0 or greater on Unix and from PHP 4.2.3 on win32. UTF-8 validity of the pattern is checked since PHP 4.3.5.

Do we need to change the following:

$delta = strtolower(preg_replace('/[^a-z0-9]/i', '-', $form_state['values']['info']));

To

$delta = strtolower(preg_replace('/[^a-z0-9]/iu', '-', $form_state['values']['info']));

If so, we may need to do so in other parts of Drupal.

Hm. David_Rothstein suggested via email that perhaps the query in block_box_get() is itself the problem. Does simplifying that query solve the issue without breaking anything?

function block_box_get($bid) {
  return db_fetch_array(db_query("SELECT * FROM {boxes} WHERE bid = %d", $bid));
}

There's a slight change in block_box_delete() needed as well:

$form['info'] = array('#type' => 'hidden', '#value' => $box['info'] ? $box['info'] : $box['title']);

should become (with some formatting improvements thrown in also):

$form['info'] = array(
  '#type' => 'hidden',
  '#value' => $box['info'],
);

But that shouldn't be a problem at all, since $box['info'] is never empty. And as far as I can tell, that's the only place in the code where the "title" column from the query ever gets used. (Which makes sense: The query occurs within hook_block, and conceptually, hook_block should never need to know anything about the main blocks table.)

I'm not at a place where I can roll a patch right now, but I could roll that change into the patch later tonight if no one else does first...

I am almost convinced that block_box_get() is an archaic function that no longer serves its former purpose.

Revised patch.

I tested this and it seems to works fine (no problem creating, configuring or deleting blocks).

It's probably good if someone else takes a look, though, to make sure we aren't missing anything obvious. Right now this patch seems a little too good to be true ;)

Well, it does occur to me that if we want to hit the goals of http://drupal.org/node/216072, then the first, more complex patch from #52 is required.

Part of the goal was to remove numeric deltas and use strings to make things easier for themers. The patch in #57 keeps numeric deltas for custom blocks.

So it's a question of requirements: do we want string deltas or not?

To me, #57 makes sense for 6.x and I think #52 is worth looking at for 7.x (for the reasons given on the other issue). Maybe we should move #52 over to there, or start a new issue for it, so that this one only deals with the critical bug?

Moving #57 over to http://drupal.org/node/252921

Putting the active patch here.

Hm, I have to admit I'm a little confused about how the issues are now broken up....

For the immediate purpose of a bugfix, we should probably focus on #57, and that should be for Drupal 7 too, right??? (If the JOIN turns out to be unnecessary, then it shouldn't be in Drupal 7, either.)

Working on #61 certainly makes sense too, but that would then be more of a feature request (as @catch suggested).

So I think what I'm trying to say here is that anyone following this issue with the immediate goal of fixing the "JOIN with mixed data types fails on pgsql 8.3" bug should definitely test out the patch in #57 and make sure it doesn't break anything, and then hopefully that one can be committed right away.

I don't think so. For Drupal 7, the goal is to remove numeric deltas. The patch here (#61 now) does that.

For Drupal 6 and lower, we just want to remove the SQL error. I already split the immediate fix into a separate issue.

http://drupal.org/node/252921

@drupallfm Then you should review http://drupal.org/node/252921 properly.

Yup, the patch at http://drupal.org/node/252921 is a two-line patch that is basically RTBC but needs one more reviewer. Please review that.

I'm retitling both these issues and marking the other one critical, so the distinction is clearer...

drupallfm - please post this information on http://drupal.org/node/252921 and mark it RTBC.

Patch does not apply anymore.

I am not entirely sure that this patch is needed, now that {block}.bid is a serial.

After reviewing the issue, the need for this patch in D7 has been removed. All that remains, potentially, is removing the numeric deltas in order to make theming easier, which strikes me as a feature, not a bug.

Attached is a patch that converts the 'info' string of the custom block into the delta for use with block css classes. The logic follows that from theme_preprocess_page(), where we strip all non-alphanumeric characters from the string.

That regex can be simplified to ![^a-z0-9-_]+!s. It's probably also better to use / rather than ! as the delimiters. Both work, but / is more common in cases where it won't conflict with the regex itself.

Hm. Then we have more patching to do, since that regex comes straight from template_preprocess_page(), which seems to be the closest core use-case to this patch.

Explanation follows:

  // Add arg(0) to make it possible to theme the page depending on the current page
  // type (e.g. node, admin, user, etc.). To avoid illegal characters in the class,
  // we're removing everything disallowed. We are not using 'a-z' as that might leave
  // in certain international characters (e.g. German umlauts).
  $body_classes[] = preg_replace('![^abcdefghijklmnopqrstuvwxyz0-9-_]+!s', '', 'page-'. form_clean_id(drupal_strtolower(arg(0))));

@agentrickard: then the only thing missing is a proper comment :)

New patch, with some docblock comments and a line explaining why we do not use [a-z].

@Crell, originally I was going to use \W, but the template_preprocess_page() code convinced me otherwise.

Ah, that makes sense. It's probably worthwhile to factor both out to a drupal_class_name() or drupal_class_safe() utility function then. More consistency, one place to document, and a nice tool to use elsewhere to boot.

New patch creates drupal_css_class_string() function inside common.inc.

There's no need to have an explanation of the regex in block_block_view() now, as the regex is over in the utility function instead.

Removed.

What happens when a site admin edits the block description? Won't that change the CSS class that is applied to the block, thereby potentially causing their site's theme to break?

(Otherwise, it looks good.)

Seeing how there's a form_clean_id, should this become form_clean_class, or something?

@David_Rothstein -- It would, which may be a good reason not to do this.

@Rob Loach -- This is not a form function. Maybe drupal_clean_css_class()?

@ #81, this isn't form-specific. Neither of the current use cases have anything to do with forms. (Arguably form_clean_id() shouldn't mention forms either, actually, since Zen theme has to have its own implementation, but that's a separate matter.)

Revised patch. Either we live with the potential issue raised in #80, or we abandon this approach.

Function renamed to check_class(), which seems in line with similar functions (like check_url) that strip unwanted characters..

Fixes the test case failure.

Could #355236: Refactor drupal_render theming - docs help this?

@Rob Loach

Uncertain. But I'll be damned if I'm going to update this patch after waiting 3 weeks for a test.

http://api.drupal.org/api/function/template_preprocess_block/7
http://api.drupal.org/api/function/drupal_html_class/7

This is fixed right?

Appears to be, yes.