when adding a new block, say in the right, and then click in the configure link, it says:

Fatal error: Unsupported operand types in /var/www/drupal-prod/modules/block/block.module on line 308

and then more warnings, concerning database issues, i.e. Inner Join in line 298 block.module and other query failed, operator does nor exist integer =character ..

regards

Files: 
CommentFileSizeAuthor
#88 safe-css-class.patch4.58 KBagentrickard
Failed: 9678 passes, 2 fails, 0 exceptions View
#85 safe-css-class.patch3.43 KBagentrickard
Failed: 8852 passes, 5 fails, 0 exceptions View
#79 safe_css_class.patch3.47 KBagentrickard
Failed: Failed to install HEAD. View
#77 safe_css_class.patch3.57 KBagentrickard
Failed: Failed to install HEAD. View
#75 block_box_delta.patch1.17 KBagentrickard
Failed: Failed to install HEAD. View
#71 block_box_delta.patch915 bytesagentrickard
Failed: Failed to install HEAD. View
#61 block-sql_1.patch6.89 KBagentrickard
Failed: Failed to apply patch. View
#57 block-box-get.patch1.56 KBagentrickard
Failed: Failed to apply patch. View
#55 block-box-get.patch685 bytesagentrickard
Failed: Failed to apply patch. View
#52 block-sql.patch6.89 KBagentrickard
Failed: Failed to apply patch. View
#51 block-sql.patch6.14 KBagentrickard
Failed: Failed to apply patch. View
#50 block-sql.patch6.14 KBagentrickard
Failed: Failed to apply patch. View
#49 block-admin.patch2.84 KBagentrickard
Failed: Failed to apply patch. View
#49 block-install.patch819 bytesagentrickard
Failed: Failed to apply patch. View
#49 system-block.patch833 bytesagentrickard
Failed: Failed to apply patch. View
#10 block_sql.patch712 bytesagentrickard
Failed: Failed to apply patch. View

Comments

agentrickard’s picture

Component: database system » postgresql database
Status: Active » Postponed (maintainer needs more info)

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?

drupallfm’s picture

I upgraded to drupal 6.0 final, and tested with postgres 8.2.5 and it worked fine.

Than change database, to postgres 8.3, and the error continues, here is the the DB log:

LOG: database system is ready to accept connections
WARNING: nonstandard use of \\ in a string literal at character 32
HINT: Use the escape string syntax for backslashes, e.g., E'\\'.
WARNING: nonstandard use of \\ in a string literal at character 122
HINT: Use the escape string syntax for backslashes, e.g., E'\\'.
ERROR: operator does not exist: integer = character varying at character 68
HINT: No operator matches the given name and argument type(s). You might need to add explicit type casts.
STATEMENT: SELECT bx.*, bl.title FROM boxes bx INNER JOIN blocks bl ON bx.bid = bl.delta WHERE bl.module = 'block' AND bx.bid = 4

drupallfm’s picture

saw this in postgres's site:

"Upgrade Warning
In order to uphold the PostgreSQL Project's very high standards for data integrity and reliability, in version 8.3 we have cleaned up some of the data type conversions ("casts"). This refactoring may cause problems for some users upgrading older applications which were written to be careless about data type comparisons, especially from versions of PostgreSQL which are several years old. Users who are upgrading very old applications, or who suspect that they may have some sloppy application or stored procedure code, should do extra testing before upgrading their production systems. See the release notes for more information.

"

agentrickard’s picture

Version: 6.0-rc4 » 6.0
Status: Postponed (maintainer needs more info) » Active

Nice report. Bumping status.

agentrickard’s picture

Title: Error with Postgres 8.3 » block_box_get fails on pgsql 8.3

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.

agentrickard’s picture

Priority: Normal » Critical

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.

agentrickard’s picture

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
agentrickard’s picture

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

agentrickard’s picture

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.

agentrickard’s picture

Status: Active » Needs review
FileSize
712 bytes
Failed: Failed to apply patch. View

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

agentrickard’s picture

Title: block_box_get fails on pgsql 8.3 » JOIN with mixed data types fails on pgsql 8.3

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

agentrickard’s picture

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).

drupallfm’s picture

Tested with the patch and worked fine!
Dont't forget to include next release drupal 6.

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

Thks for the solution, I will continue to test this plataform , drupal 6 + pgsql 8.3 and if I detect more errors (hope not!) I will report.

agentrickard’s picture

@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.

agentrickard’s picture

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.

drupallfm’s picture

Status: Needs review » Reviewed & tested by the community

Thats right, better be the only SQL statetment.

patch ready to be committed.

drupallfm’s picture

Thats right, better be the only SQL statetment.

patch ready to be committed.

agentrickard’s picture

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.

RobLoach’s picture

Version: 6.0 » 7.x-dev
Status: Reviewed & tested by the community » Needs review

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

agentrickard’s picture

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

hswong3i’s picture

Subscribe.

bjaspan’s picture

subscribe

chx’s picture

Status: Needs review » Needs work

I thought we are going with ANSI SQL.

agentrickard’s picture

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

Gijs’s picture

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

Thx

Gijs

agentrickard’s picture

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.

phayes’s picture

Ran into this issue running drupal 5.7

Applied patch - works great

Dries’s picture

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

ajg112’s picture

Version: 7.x-dev » 5.7

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

agentrickard’s picture

@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.

Crell’s picture

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.

agentrickard’s picture

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

Crell’s picture

@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.

drumm’s picture

Version: 5.7 » 7.x-dev

Moving back to the most recent affected version.

linuxpoet’s picture

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.

Crell’s picture

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.

agentrickard’s picture

@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.

Crell’s picture

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.

agentrickard’s picture

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

chx’s picture

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

agentrickard’s picture

@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....

catch’s picture

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.

agentrickard’s picture

@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.

Crell’s picture

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.

catch’s picture

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?

agentrickard’s picture

/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.

Crell’s picture

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.

agentrickard’s picture

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

agentrickard’s picture

Status: Needs work » Needs review
FileSize
833 bytes
Failed: Failed to apply patch. View
819 bytes
Failed: Failed to apply patch. View
2.84 KB
Failed: Failed to apply patch. View

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.

agentrickard’s picture

FileSize
6.14 KB
Failed: Failed to apply patch. View

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.

agentrickard’s picture

FileSize
6.14 KB
Failed: Failed to apply patch. View

Fixes a comment typo.

agentrickard’s picture

FileSize
6.89 KB
Failed: Failed to apply patch. View

Fixes missing %d to %s conversion.

David_Rothstein’s picture

subscribe

agentrickard’s picture

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.

agentrickard’s picture

FileSize
685 bytes
Failed: Failed to apply patch. View

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));
}
David_Rothstein’s picture

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...

agentrickard’s picture

FileSize
1.56 KB
Failed: Failed to apply patch. View

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

Revised patch.

David_Rothstein’s picture

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 ;)

agentrickard’s picture

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?

catch’s picture

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?

agentrickard’s picture

FileSize
6.89 KB
Failed: Failed to apply patch. View

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

Putting the active patch here.

David_Rothstein’s picture

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.

agentrickard’s picture

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’s picture

As said before, what is important for now, is to implement a quick fix (the patch) in the drupal 6.3, to make the upgrade process more simple.

thks

agentrickard’s picture

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

David_Rothstein’s picture

Title: JOIN with mixed data types fails on pgsql 8.3 » Remove numeric deltas from the boxes table (also fixes broken JOIN in pgsql 8.3)

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’s picture

Tested the:

#3
David_Rothstein - May 20, 2008 - 15:18

I attached the patch and then it disappeared... so here it is again ;)

Attachment Size
block-box-get.patch 1.56 KB

And it worked, everything seems ok now.
(and the sql statement appears to be more "cleaner" and optimized)

Thks

catch’s picture

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

R.Muilwijk’s picture

Status: Needs review » Needs work

Patch does not apply anymore.

agentrickard’s picture

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

agentrickard’s picture

Category: bug » feature
Priority: Critical » Normal
Status: Needs work » Needs review
FileSize
915 bytes
Failed: Failed to install HEAD. View

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.

Crell’s picture

Status: Needs review » Needs work

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.

agentrickard’s picture

Status: Needs work » Needs review

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))));
Damien Tournoud’s picture

Title: Remove numeric deltas from the boxes table (also fixes broken JOIN in pgsql 8.3) » Ensure block delta is class safe
Component: postgresql database » block.module

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

agentrickard’s picture

FileSize
1.17 KB
Failed: Failed to install HEAD. View

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.

Crell’s picture

Status: Needs review » Needs work

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.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
Failed: Failed to install HEAD. View

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

Crell’s picture

Status: Needs review » Needs work

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.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
Failed: Failed to install HEAD. View

Removed.

David_Rothstein’s picture

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.)

RobLoach’s picture

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

agentrickard’s picture

@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()?

Crell’s picture

@ #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.)

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

FileSize
3.43 KB
Failed: 8852 passes, 5 fails, 0 exceptions View

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..

agentrickard’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
4.58 KB
Failed: 9678 passes, 2 fails, 0 exceptions View

Fixes the test case failure.

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

agentrickard’s picture

@Rob Loach

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

casey’s picture

agentrickard’s picture

Status: Fixed » Closed (fixed)

Appears to be, yes.