Change format into string

Just to state the obvious: you need an upgrade path for (text) fields.

And it would have been *way* easier to do that *before* the beta.

sub

Re #5: Yes. See #933846: Better distro support for all the messy background. :-)

I see two possibilities for the upgrade path:

Ints are still valid strings. There's, programmatically, nothing about "1" that makes it invalid as a string identifier. We could just leave it. :-) An advantage of this approach overall is that, because ints are valid strings, the API breakage is kept to an absolute minimum.

As an extension of that, simply add a prefix. format 1 becomes "format_1", format 2 becomes "format_2", etc. I believe coder upgrade module did that for block deltas, which also switched from mostly ints to must-be-string in D7.

+++ modules/block/block.install	7 Oct 2010 02:21:14 -0000
@@ -155,8 +155,8 @@ function block_schema() {
-        'type' => 'int',
-        'unsigned' => TRUE,
+        'type' => 'varchar',
+        'length' => 64,

+++ modules/filter/filter.admin.inc	7 Oct 2010 02:30:54 -0000
@@ -122,6 +125,16 @@ function filter_admin_format_form($form,
+    '#type' => 'textfield', // @todo http://drupal.org/node/902644

While I cannot think of a use case off hand for a machine name longer than 64 characters, we've been bitten by too-short varchar fields in the past many many times. Especially if we end up with dynamically created machine names (which will happen sooner or later; it always does). We probably want to make this bigger and save ourselves the headache later.

+++ modules/field/modules/text/text.test	7 Oct 2010 02:26:21 -0000
@@ -386,8 +389,15 @@ class TextTranslationTestCase extends Dr
-    $this->format = 3;
-    $this->admin = $this->drupalCreateUser(array('administer languages', 'administer content types', 'access administration pages', 'bypass node access', "use text format $this->format"));
+    $full_html_format = db_query_range('SELECT * FROM {filter_format} WHERE name = :name', 0, 1, array(':name' => 'Full HTML'))->fetchObject();
+    $this->format = $full_html_format->format;

If the idea is to have a machine name, why is this being queried by human name?

+++ modules/filter/filter.module	7 Oct 2010 02:21:14 -0000
@@ -192,7 +192,8 @@ function filter_format_save(&$format) {
   // Add a new text format.
-  if (empty($format->format)) {
+  $exists = db_query_range('SELECT 1 FROM {filter_format} WHERE format = :format', 0, 1, array(':format' => $format->format))->fetchField();
+  if (!$exists) {
     $return = drupal_write_record('filter_format', $format);
   }

This seems like a potentially good case for a merge query instead.

Powered by Dreditor.

Ints are still valid strings. There's, programmatically, nothing about "1" that makes it invalid as a string identifier. We could just leave it. :-) An advantage of this approach overall is that, because ints are valid strings, the API breakage is kept to an absolute minimum.

In the D6 world, this is what we're looking at in moving Workflow module from serial ids to machine names for its workflows. I'm partial to the "Just turn the ID into a string" approach because it utterly eliminates any danger of stray ids being scattered around and missed by the upgrade path.

As an extension of that, simply add a prefix. format 1 becomes "format_1", format 2 becomes "format_2", etc. I believe coder upgrade module did that for block deltas, which also switched from mostly ints to must-be-string in D7.

This is purely aesthetic, though, isn't it? There's nothing (in theory) to keep us from just using the numeric ones. That's what was also done for node build modes in previous versions of Drupal -- while it was less than ideal, we demonstrated that contrib can build on that effectively.

I've read and applied the patch in #2. It works as expected and makes working with filter formats in code more readable (see adjustments to tests and use of API to create filter formats). This is a much cleaner approach to introducing machine names into core than adding an additional column - it means contrib can continue to use the same ->format property and have a minimal upgrade path that involves at most a small schema change (like the changes in patch to taxonomy.install) or no changes at all if format references are stored flexibly in a serialized field or a variable.

Other than Crell's notes in #8, this patch will need an upgrade path which I am happy to help with and test.

Eaton: Actually, if anything I think the node build modes in D6 is a great example of why we should convert to strings. :-) To this day I am still mystified by what I can do with D6 build modes and do not comprehend why I sometimes get different lists.

No, db_merge() doesn't handle objects. It's a simple cast, though. And if a field isn't defined, the DB should fill in the default anyway, shouldn't it? d_w_r() just pulls its defaults from the schema anyway. Even then, are we actually going to get partially-filled $format objects in practice (that don't count as broken code to begin with)?

I admit I'm somewhat biased since I consider d_w_r() to be a blight upon the earth to be exterminated with religious zeal; its only reason to exist is that db_merge() didn't exist yet as of Drupal 6 and we haven't fully gotten around to expunging its evil from the world.

Possibly unrelated here, but I also note that the filter table has a field (that's being modified here) that is marked as being a foreign key in the description but doesn't have its FK-ness defined in the schema API. We should fix that.

"- I've removed the unique index on {filter_format}.name. filter.admin.inc still contains form validation logic for that uniqueness. Also, the filter_update_N() does not remove the index."

That doesn't make sense to me. There are other ways to create formats than through the UI. What problem does removing this index actually solve?

db_merge() followed by a select would do it, wouldn't it? :-) It's the same number of queries.

I agree with webchick and don't grok sun's response. Format should be a sole primary key in its table. Right now, it looks like (format, name) is the primary key. Duplicate machine names should be enforced at DB level in addition to FAPI.

If we *also* want a unique index on name, thats fine.

Overall, I like the approach taken in this patch. It's not as bad of an API change as I expected it to be so I think we should seriously consider this for Drupal 7. Let's continue to flush out the remaining glitches.

Although I have stopped doing D7 work because I was called a problem and until I get an apology for that I am not going to do anything, I had enough of sun's allegations of how field storage engines are badly designed. Dude. We did not design it in a basement closed away from the world and then locked it down for good. Quite the opposite. Even when we were sprinting we were giving reports often. And after that you had two years to contribute and speak up if you had a problem. We made it crystal clear that the divisor between fields and instances is that fields get the things that are hard to update. Like, you know, the schema.

Now, the main argument against keeping the rules as they stand for everyone is that we do not have an export-import tool that can handle variables? Create a text format, save the id into a variable and then use that variable to create, say, a view. As what you want is a distro, this is perfectly doable as you know your own variables. This can be done without doing a schema change past beta.

Here is a list of comments from the other issue who said this is Drupal 8, just so that we see clearly

1. Dave Reid, twice http://drupal.org/node/933846#comment-3540604 and http://drupal.org/node/933846#comment-354065
2. heyrocker http://drupal.org/node/933846#comment-3541318
3. mfer http://drupal.org/node/933846#comment-3542502
4. fago http://drupal.org/node/933846#comment-3545144
5. catch http://drupal.org/node/933846#comment-3546314

+ foreach (module_implements('field_update_forbid') as $module) does not belong in an update function.

The human-readable name of a format is unimportant with this patch. Until now, we tried ;) to enforce some kind of uniqueness for text format labels, but those can be changed at any time, so it's like renaming a node type's label. Completely unnecessary if there is a unique machine name.

I once thought so too...

But then was educated otherwise: http://drupal.org/node/11218#comment-1986086

Those proposed changes are a separate issue and should not be part of this patch.

Now, this became a kitten killer monstrosity. You are changing which keys are unset field_update_field, adding a $conditions mechanism for field_update_field, creating a problematic and controversial _update_7000_field_update_field utility, changing field_sql_storage_field_update_forbid / field_sql_storage_field_storage_update_field, the accompanying test. http://www.webchick.net/please-stop-eating-baby-kittens

No. File different issues. This is completely and absolutely unreviewable. I would be glad to work with you to get field API problems fixed in a speedy manner but not in one issue which, in itself, is not yet approved by Dries to be Drupal 7 anyways.

#44 is broken out to #937554: Field storage config entities 'indexes' key.

chx suggested to write a field_sql_storage-engine-only update hook for changing the field schema for format. Let me give this a shot.

This is patch #44 with the following modifications:

- Implement text_update_7000() for SQL storage engine only (this removes the entire _update_7000_field_update_field($prior_field, $field) portion of #44).
- Use form element #type 'machine_name' after #902644 went in.

Note that we'll still need update-friendly versions of field_update_field() / field_update_instance(). There will be housekeeping of stored $field and $instance definitions. Those could possibly be just the 'array-massaging-into-SQL-friendly-structure' part of their 'official API' counterparts - that is, once those very parts are sorted out in #937554: Field storage config entities 'indexes' key.

Not necessary for this patch if it can live without, though.

#50 looks good to me on visual inspection. I defer to the Field API experts on #51. Nice use of machine_name, too.

Subscribe

Decided to commit this patch to help us improve support for distributions.

Thanks for working on this, for trusting my judgment and respecting the decision.

Let's continue to make progress, please.

Wow, amazing.

Created issue #946876: Filter format IDs are now strings! Use descriptive IDs in Filter Example for examples project to utilize this. It seems more than half of D6 code that provides blocks uses an int for the delta, even though it became string all that long ago. We really don't want that to happen with filter IDs so we should promote the new use aggressively.

I think this should probably be announced too, even though it doesn't break anything.

OK, tell me that after linking http://drupal.org/node/11218#comment-1986086 how did we commit dropping human name unique index?

Each text format human-readable name should be unique, as it's the resource that users have to identify them among others text formats. Having several text formats with the same human-readable names is a potential security risk, will be easy for users and admins to forget what is each text format intended for, what and configuration should be applied to each. On the final user side, how will them know which text format to use if they have exactly the same name.

Not only this but the schema definition wasn't updated to remove the unique key, it was only dropped in the update.

This is precisely the kind of fallout that allowing schema and changes past beta creates, as the original issue spawns more and more followups.

Since it has only been a few hours since this was committed, let's hope no-one ran the update yet and just remove that hunk. If people care about the few hours window, they can open a head2head issue.

If people were feeling helpful, they could open an issue for mongodb_field_storage module for this too - mongo doesn't need a schema change, but it's type strict, so it's likely all the int values in mongo storage will need converting from text to string types in place, otherwise queries on the wrong type will fail.

That sounds a plan. Those who want that unique dropped can open another issue because it does not belong to this one in the first place!

In #35 the hook_schema() change and the update hook were both in, in #36 the schema change was reverted.

So it looks like sun forgot to revert the update hunk, and then everyone was so excited about distributions that they didn't notice in the 11 days between the patch being posted and committed. Which makes this no less annoying but I don't think there's anything else needs to be done apart from removing that hunk and closing this issue again as quickly as possible.

Committed #58 to HEAD.

Needs to be documented in the module upgrade guide, since it affects any module that stores text format data.

Looking at the patch, this also seems like it broke the upgrade path for taxonomy.module - sites upgrading from D6 (or D7 beta 1) will still wind up having {taxonomy_term_data}.format be an integer, won't they?

Sorry to spoil the party but "Commit #438848 by Dries at 03:15" including changes to "Drupal core: /modules/field/modules/text/text.install 1.3" fails on running update.php with a "DatabaseSchemaObjectDoesNotExistException", see attached screen shot.

dev version before this one was from Oct-18

It's above my head but though to let you know it breaks, possibly because I missed a dev version in between?

[edit : crosspost with #64]

The D7->D7 upgrade is also broken.
text_update_7000() raises an exception in DatabaseSchema_mysql::changeField() :
"Cannot change the definition of field field_data_.comment_body_format: field doesn't exist."

That's most probably because of :

foreach ($fields as $field_name => $field) {
  $table = _field_sql_storage_tablename($prior_field);
  $revision_table = _field_sql_storage_revision_tablename($prior_field);

$prior_field is undefined - should be $field.

AAMOF, I'd recommend hardcoding table names in the update func. If for some reason field_sql_storage changes its table naming convention in the future, the _field_sql_storage_tablename() function will return a table name that does not match the state of the db at the time the text update is run. We had similar cases in CCK, and had to insert schema version checks inside the tablename() functions back then.

The fix for text_update_7000() is good - tested, works fine.

I'll let David feedback on the taxo update fix.

#67 works for me, thanks.

Committed #67 to HEAD. Hopefully that makes testbot a bit happier. And this is why we shouldn't make schema changes during beta. :D

Reverting back to "needs work" for documentation.

Those upgrade path changes should have been caught by tests. Or was the test bot actually down?

Great to see this committed. Sorry for the fallout.

Just talked to sun, I will supply tests and upgrade documentation shortly.

I'll let David feedback on the taxo update fix.

It looks fine to me, and I tested that it works also.

At least for this one, I think it would be hard to write an upgrade test that catches it. There is no error on upgrade (just a different schema) - you wouldn't see anything actually explode on your site until you (a) created a new text format, and then (b) assigned it to a taxonomy term description.

The text.module update issue, though, yeah - that could probably have a test.

Documentation requested in #62 is up: http://drupal.org/node/224333#text_format_string

Adds upgrade tests for

- filter_format.format
- user.signature_format
- block_custom.format

My questions:

- Did I add the test in the right place (simpletest/tests/upgrade/upgrade.filter.test)?
- What other upgrades should be tested?

I spent some more time thinking about the committed patch, and there are a couple more issues here, including another security problem.

1. There is some out-of-date documentation, particularly for filter_format_save(). Fixed in the attached patch.
2. When creating a new text format, the machine-readable name is validated using filter_format_load() which only returns enabled formats, not "disabled" (a.k.a. deleted-but-still-hanging-around-the-database) ones. That is a bug, and although somewhat of an edge case, it leads to a security issue as follows:

   Suppose you have a text format that is used on your site, and there is content stored with it. Then you disable the format. Now you go create a new text format and decide to give it the same machine-readable name as the old one had. Boom. Instead of creating a new format, what happens is that the old disabled format is "turned on" again, and is given all the properties+filters that the old one had. So all your content starts displaying again, but in a completely different format. That is a security hole.

   The attached patch fixes that by adding a new filter_format_exists() function, although:

   • It could certainly use a test.
   • A better fix might be to change the behavior of filter_format_load()... but that would just be adding API changes on top of API changes at this point.

Oops, crosspost.

Here are the two patches merged together. Could still use a test for the security issue I mentioned above, though.

On a brighter note, here is a nice cleanup issue that the original committed patch here allows :)

#947844: Clean up filter-related tests that load text formats by their human-readable name

Adding a test for #76

- Added a test attempting to create a format of an existing machine name.

I see one existing assertion of the filter tests failing on my local *vanilla* D7 installation ('Filter removed invalid tag.'), let's see what test bot says.

- Fixed checking for existing filter format _names_ (this was broken due to a check for the presence of $form_state['values']['format'] - which now is always present).
- Added a test verifying that filter formats of the same name can't be created.

Per discussion w/ chx on IRC, I've improved the following comments:

- Compare functionality between filter_format_load() and filter_format_exists() in filter_format_exists() comment.
- Comment on format_set_value() in filter_admin_format_form_validate().
- Improve comments on assertions verifying that it isn't possible to create duplicate machine names / human readable names even if text formats in question are disabled.

Looks good!

I'd love to see a final +1 here from sun or David Rothstein, but looks good here. Thanks for the quick rally.

Tagging as beta blocker because it'd be good to get this new feature completely polished off beforehand.

Looks mostly good, and nice catch on the isset() thing in filter_admin_format_form_validate()... It seems there is a similar isset() check in filter_admin_format_form_submit() that could also be removed? But it doesn't have to be done here.

The first new filter module test looks good to me, but I don't understand the second one:

+    // Attempt to create a format of the same human readable name as the
+    // disabled format but with a different machine name.
+    $edit = array(
+      'format' => 'new_format',
+      'name' => $format->name,
+    );
+    $this->drupalPost('admin/config/content/formats/add', $edit, t('Save configuration'));
+    $this->assertText('Text format names must be unique. A format named ' . check_plain($format->name) . ' already exists.');

As far as I know, that behavior is a bug to be fixed, not something we should be verifying in a test :) ... (See the followup discussion in #914458: Remove the format delete reassignment "feature" - which I will move to its own issue after writing this.) If the format has been disabled, it is completely gone from the user interface, so we don't have any reason to prevent people from trying to create a new one with the same name.

David, so how should we proceed? Remove that test?

Yeah, I would say remove it. Either that or modify it, so that the test tries to reuse the human-readable name of an enabled format instead. (Since that restriction is something we definitely do want to enforce.)

- Removes the stray isset() that David mentions in #86

Re: #86 test for unique human readable names: Let's keep this test in, as it verifies existing functionality - even though the functionality itself may be questionable. When #914458 lands, this test will need modification - great way to ensure test coverage =)

#90: drupal.format-exists-update.90.patch queued for re-testing.

#90's bot hiccup has been happening a lot lately :(

+function filter_format_exists($format_id) {
+  $formats = db_select('filter_format', 'ff')
+    ->fields('ff')
+    ->condition('format', $format_id)
+    ->execute()
+    ->fetchAllAssoc('format');
+
+  return isset($formats[$format_id]);
+}

I don't understand this change. Why do we want to load the entire contents of that row just to check if the row exists? If that behavior is needed for #903730: Missing drupal_alter() for text formats and filters, then in my opinion it should be discussed there, not here.

#89 looks good to me, though. Yeah, I guess I agree with @alex_b's rationale about keeping the second test in. It will indeed ensure that we remember to write new tests if/when that particular bug is fixed!

Yes I don't see what's wrong with #89 or why non-critical feature requests need to hold this up.

David's concerns have been addressed, the last patch is by sun, again addressing problems -- seems we are good to go. I attached an interdiff of #88 - #95.

Committed #95 to HEAD.

I'm going to mark this closed now. I hope that it stays that way. :D

Reverting status + title, I think.

Schema module is throwing a warning: filter.format is part of the primary key but is not specified to be 'not null'. Sorry I didn't yet read thru this issue, but is there some reason filter.format 'not null' changed from TRUE to FALSE?

*sigh*

If it's a beta blocker, it's also critical, right?

.

Given that MySQL changes NULL fields without a word this, we were already running with this patch applied so it does not require a review.

mysql> create table foo (a int null, b int null, primary key(a,b));
Query OK, 0 rows affected (0.03 sec)

mysql> show create table foo;
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                                |
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------+
| foo   | CREATE TABLE `foo` (
  `a` int(11) NOT NULL DEFAULT '0',
  `b` int(11) NOT NULL DEFAULT '0',
  PRIMARY KEY (`a`,`b`)
) ENGINE=MyISAM DEFAULT CHARSET=latin1 |
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

effulgentsia asked me to patch the update too. That's totally unnecessary but for the sake of consistency I did that too. Once again: this patch does NOT change your database at all because MySQL makes your primary key NOT NULL whether you want or not.

Looks good. The schema was already silently corrected by MySQL. With this patch, the declared and actual schemas match so schema.module is happy.

Hm. The original patch has a number of update functions, each of which are setting format to 'not null' => FALSE. Methinks they should be updated as well.

To confirm this, we'd need to run schema module on a database upgraded from beta1 to HEAD.

Ok, I get it.

It's problematic here because it's a PK field. In other modules' schemas it won't be.

I committed #105 to HEAD to remove the error. My hunch is we haven't seen the last of this issue yet, though, as not null -> FALSE seems to be a deliberate decision made for some reason up above.

Although as effulgentsia pointed out on IRC moments ago, this could've just been a copy/paste oversight since this is the "source" table for these formats in the first place, and can never be NULL.

I guess time will tell if we've heard the last of this issue or not. :)

'not null' => FALSE in the other tables is probably correct, since it's not a primary key in those tables. It was likely a copy/paste error to have 'not null' => FALSE in the {filter} table itself, and not caught, given that MySQL silently corrected it. Given #106, RTBC.

x-post

I really hate to do this. But.... The test from #95? It was never committed :)

#111: oh_no_reopened_again.patch queued for re-testing.

D'oh. Thanks.

Committed to HEAD. Really, really marking fixed now. :D

Eight months later I am tempted to open this again, since it introduced a fatal error in the upgrade path for sites that had CCK module installed in D6. However instead of doing that I opened #1228488: Drupal 7 text module update (added during beta) runs on sites that have D6 CCK text module installed.

http://drupal.org/node/934050#comment-3552204 still stands.

So tempted, I actually did re-open it, moving back.

Note that #50 was committed to core after basically a single review by sun who wrote most of it. And it broke the upgrade path? News at 11! Yay for breaking the process and the freeze!