Text format loses weight and status when saving settings

+++ modules/filter/filter.admin.inc	20 Nov 2010 20:30:29 -0000
@@ -308,8 +308,8 @@ function filter_admin_format_form_submit
-  // Save text format.
-  $format = (object) $form_state['values'];
+  // Save text format. Use the original values for data that is not submitted.
+  $format = (object) ($form_state['values'] + get_object_vars($form_state['build_info']['args'][0]));

This needs to be reverted. Instead, we need to put 'weight' and 'status' as #type 'value' elements into the filter_admin_format_form(). Ideal location is right below the 'format' element (before the roles).

Can you quickly re-roll with that?

Powered by Dreditor.

And we need tests. And we cannot release with this bug, since users are potentially hi-jacking their site without knowing.

Ideally, we commit #947844: Clean up filter-related tests that load text formats by their human-readable name first.

Tests.

Plus fix.

Minus commented out test.

This is one of the issues currently marked critical that are actually critical. It'd be nice to get this resolved asap.

Just trying to get up to speed so bear with my stoopid questions:

- Not sure why this is critical. So the order of the formats changes for a text area. Not a big deal. AFAICT this bug does _not_ change the default. And could therefore be downgraded to "major" if not "normal"?

- @sun why $form_state['redirect']? It doesn't seem to have anything to do with this bug.

- All over Drupal core we seem to be mixing how we use "status". In some places we use constants (TRUE, FALSE, NODE_NOT_PUBLISHED, NODE_PUBLISHED, etc.) and in other places we just use integers. IMO they should all be constants.

The latest patch no longer applies (due to the tests). I'll try and figure out what is going on and re-roll.

Ah, I see that it does affect what filter is default :P

The first format available to a user will be selected by default.

So we should keep at critical.

The best that I can determine is that @sun was mixing fixes for two different issues in his last patch. My patch here is what I _think_ was the intent.

Per IRC, changing security-applicable tag.

+++ modules/filter/filter.admin.inc	29 Nov 2010 08:42:03 -0000
@@ -104,6 +104,8 @@ function filter_admin_format_page($forma
+      'status' => TRUE,

@@ -140,6 +142,14 @@ function filter_admin_format_form($form,
+    '#value' => $format->status,

+++ modules/filter/filter.module	29 Nov 2010 08:42:03 -0000
@@ -173,6 +173,8 @@ function filter_format_load($format_id) 
+ *   - 'status': (optional) A Boolean indicating whether the text format is
+ *     enabled. Defaults to TRUE.

$format->status is an integer for stored formats, so we should use 0/1 in order to not confuse other modules seeing sometimes 1 and sometimes TRUE and sometimes 0 and sometimes FALSE.

Additionally, if a contributed module was actively exposing the format's status as a checkbox on the administration form, then its return value would be 0/1, too.

+++ modules/filter/filter.test	29 Nov 2010 08:42:03 -0000
@@ -186,21 +186,34 @@ class FilterAdminTestCase extends Drupal
-      'format' => drupal_strtolower($this->randomName()),
-      'name' => $this->randomName(),
+      'format' => $format_id = drupal_strtolower($this->randomName()),
+      'name' => $name = $this->randomName(),

This inline assignment of variables won't fly. $format_id and $name need to be declared upfront then.

+++ modules/filter/filter.test	29 Nov 2010 08:42:03 -0000
@@ -186,21 +186,34 @@ class FilterAdminTestCase extends Drupal
+    $this->drupalPost('admin/config/content/formats', $edit, t('Save changes'));
+    $this->drupalGet('admin/config/content/formats');

The drupalGet() is not needed.

Powered by Dreditor.

@Stevel: Additionally, I'm not sure how you were able to run into the "status lost" bug. I (and the tests in my earlier patch) tried to reproduce whether it's possible to hi-jack a format's status, but without any luck. You basically outlined how to reproduce the weight bug only, but since disabled formats do not appear in the (core) UI...

Of course, we're going to fix both with this patch, but I currently see no way how to test the status bug.

I don't see what makes this a critical bug. This isn't Drupal 6, where switching default formats can open up a security hole. In Drupal 7, it should be a harmless operation.

Still an ugly bug that's worth fixing, of course.... But I'm downgrading to major. (If there actually is a critical security issue I'm not thinking about, then the tests should be changed to explicitly verify that, because it's not obvious what it is.)

Looking at the patches, the approach in the first patch actually seemed more robust to me... instead of making sure every property we need is somewhere on the form, why not just guarantee we are starting with a complete format object before saving it? (The $form_state['build_info']['args'][0] stuff is confusing though; it would probably be better to use filter_format_load() explicitly to get the format object.)

the approach in the first patch actually seemed more robust to me...

We do not do something like that anywhere else in Drupal core (or if we happen to do it somewhere, then that code is old and ugly). $format is an object with defined properties. The loader gives you a complete format object, the saver expects and requires a complete format object. When creating a new stub format object, then all properties have to be defined to make it complete. Form callbacks get a complete object. And lastly, when passing a complete object into a form, then it's expected that a complete object comes out of that form, too.

Fixed all remaining issues.

My thoughts on integers vs. boolean/constants is that 0, 1 implies that other values are possible (3, -1, etc.). While TRUE, FALSE, or constants makes it clear that these are the only options that we are expecting.

I agree with Sun's approach to keeping full objects throughout the entire workflow. In other places we do sometimes work with only partial objects (partial nodes for example), but only when necessary, and it is well marked when we do.

OK, I do agree we should use the form API to pass along the object (rather than loading it separately), but putting things in $form_state['values'] one by one is not the way to go. It makes the code too hard to follow, and also very fragile.

I am working on a compromise patch which I'll post in a second.

The loader gives you a complete format object, the saver expects and requires a complete format object.
.....
I agree with Sun's approach to keeping full objects throughout the entire workflow. In other places we do sometimes work with only partial objects (partial nodes for example), but only when necessary, and it is well marked when we do.

Just to clarify, we are not working with full format objects here at all. Try running the following code on your site:

define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
$format = filter_format_load('filtered_html');
filter_format_save($format);

Your Filtered HTML format will be nicely destroyed.

(That's a separate bug, I suppose, but the point is that the filter module does not currently have a unified concept of what a complete format object consists of, so I think it is better if we stick to using the API rather than trying to individually enumerate each property that constitutes a "format" in another place in the code - in particular in a form builder function, of all places.)

Let's try the attached patch.

Explanation: I looked into what new code in D7 is doing in these kinds of situations, and the "state of the art" seems to be to put the object in $form_state directly. For example, the node form puts it $form_state['node'], and the user form puts it in $form_state['user']. That's useful because it's still available if the form is rebuilt, in addition to being available in the validation/submit handlers. In the long term, we probably want to use that method here.

However, lots of other code in core currently uses something like $form['#object'] instead (in some places with a note to remove it in D8, in other places not). Most important, that's what the Filter module itself is already doing. It does so in filter_admin_disable() and then uses that information in filter_admin_disable_submit(), and it even already does it in filter_admin_format_form(), which is the form we are working with here!

So the minimal fix for D7 should really just be to use the existing $form['#format'] we already have, and that's what the attached patch does. Then maybe we could move to something like $form_state['filter']['format'] in Drupal 8.

Ah, ok. I must've misunderstood. I thought it would be possible for filter.module to silently switch the default input format on you without your consent, which could introduce major security problems.

Nevertheless, it's a good bug to fix regardless of its criticalness. :)

Committed #23 to HEAD. Thanks!

Ah, ok. I must've misunderstood. I thought it would be possible for filter.module to silently switch the default input format on you without your consent, which could introduce major security problems.

The bug did make that possible, but there still wouldn't be any security problems in D7. (Because switching the default format doesn't grant permission for the whole world to use it, like it did in Drupal 6; all it does is affect which one is preselected on the form when someone who already has access to that format is creating new content.)

In any case, the bug is all fixed (and closed) now - just wanted to clarify that :)