White-list approach on form checks

I was also thinking of making the white-list configurable. There's bound to be situations where a local site has local/custom forms which they still want to allow submissions for if they only query data.

Or would you prefer to avoid that approach?

The attached patch is the sort of approach I was thinking. There might be other forms which we should allow by default.

Also worth noting this is a functional change to the current module approach and might restrict forms which users otherwise expect to be available, only login, search and the node content list are currently allowed, but i'm sure there's many more we should also allow.

Initially I tried to address the form validation separately, however a little bit of refactoring was needed in order to address that and also this item.
The patch addresses the following: (let me know if you would rather these were broken out into separate patches, i've included them both together as they were touching the same code so seemed more complicated to try and split apart)

The patch:
1. assumes bug patches submitted for #1883258: Errors generated when accessing forms in read-only mode and #1883310: Notice message not displayed to users have already been applied. (to avoid clashes assuming those bug patches are okay)
2. addresses #1882266: Add read-only mode checking on form submission (as that needed a small code refactor anyway to avoid duplicating code)

If you want any changes to these or anything submitted differently, just let me know.

As i'm primarily developing on D6 for the project i'm currently working on, I've included a D6 patch too, as it's subtly different with the #markup change. (again it assumes the patch for bug #1883258: Errors generated when accessing forms in read-only mode has been applied) Hopefully if you accept this, you would also consider patching the D6 branch too if I provide the patches?

This enhancement patch has been sponsored by Haymarket Media Group (http://www.haymarket.com/)

Many thanks,
Pete

Actually, on closer inspection of the actual patch file it doesn't appear to be quite as I expected, and won't patch cleanly after the mentioned bugs (#1883258: Errors generated when accessing forms in read-only mode and #1883310: Notice message not displayed to users) have been applied. Sorry about that. I used 'git diff' to produce the patch and hadn't spotted it.

Attached a corrected patch.

I originally reported this as a 6.x feature request, but this patch is for 7.x as requested as the main development release.
Will provide a 6.x patch once confirmed whether this 7.x approach is suitable.

Attached is a patch against current 7.x-1.x-dev which contains the suggested enhancements for your consideration towards the white-list idea.

In addition to the last patch, this patch includes slight refactor improvements to the code submitted in the previous patch, and in particular adds support for admin configuration of allowed forms, so that site administrators can more easily configure the module as needed. (thereby hopefully avoiding too much of an issue should we have any missing forms which ideally should be in the allowed list)

Let me know if this is the sort of thing you were thinking, whether it's suitable or anything else needed?

Hi pandaeskimo,

Great news, it would be great for more to test this feature enhancement to see whether it would benefit everyone.

Sorry for the confusion with the patches, as you can see there was a flurry of activity towards this this week with various things, and BarisW has already kindly patched the dev release with a couple of the bug patches I submitted.

All you need do to get this working is apply the single patch contained in comment 6 against the 7.x-1.x-dev release. You will then have the enhancement i've implemented. It has the ability to specify form names so you can add forms you wish to allow as exceptions to the read-only mode, which may help solve your situation.

If the patch doesn't apply/work for any reason then please let me know and i'll investigate.

Many thanks, and I look forward to your feedback and suggestions,
Pete

Hi pandaeskimo,

Perfect! The wildcard option was the one remaining item I was thinking of implementing, but as I didn't need it just yet I avoided implementing something I wasn't going to be able to fully test before supplying a patch. Plus I wasn't sure when starting to submit this patch whether it would gain any momentum.

Thanks for doing that. The code changes you have made look great.

Hi BarisW, i've been through and ported this to D6, and in the process have sanity checked things. A couple of questions:

1. user_pass form has been added as an allowed form. Using the DTAP example from the README.txt this means users can change their password during read-only mode, but would get reset back to their original password. This seems like it will confuse users. Better to not allow password resets during this time? Or, we implement with an additional state which presents a warning message saying that any changes made may be reset etc but allow them to submit anyway.

2. In readonlymode_check_form_validate it has changed to a drupal_set_message call, which means the form will still submit okay. It needs to be form_set_error so that it fails validation and therefore prevents the form submission.

Attached is a small patch to address 2 and also move the $viewonly preg_split line into the IF statement for performance reasons. Plus tidied up a comment.

I've taken the 7.x-1.x-dev code which is related to this issue and have ported it for 6.x-1.x-dev.

I've kept everything done to date in 7.x-1.x-dev related to this except for the permissions specific items #1888550: View Forms Permissions, as I should really patch that separately once tested and agreed final. (plus we may decide there's no need in backporting that one as may be deemed D7 specific?)

Update: I should have also mentioned, the backport includes the doc updates too, so we keep everything aligned.

A further tidy up of the regex matching code so that we don't have code duplicated.

This patch is to be applied after comment 19 and 20 patches are applied. To be applied to both 6.x-1.x-dev and 7.x-1.x-dev. (tested patch against both and should apply fine)

Hey,

The only official way i'm aware of for ensuring the Form API validation handler flags a validation error with the form thereby not allowing the form to complete its submission is by using the form_set_error function. I'm not seeing the double error message any more, though perhaps my testing cases are slightly different. I'm not 100% sure which of the messages you are seeing twice, could you clarify?

I must admit I normally avoid calling drupal_set_message during a form/form_alter function as it often ends in doubling up of messages during form validation. The form is parsed twice, once to validate the inbound submission, and then again when the form is generated for display. If you add any kind of AJAX into the form then that adds to the messages too. (though adding the FALSE as the third argument does solve this)

The error in D7 that I am currently seeing is the following. When loading a form (eg node/add/page) as a normal user when the site is in normal mode, then switch the site to read-only, when the user submits their form I get the following errors:

Notice: Undefined index: form_token in drupal_validate_form() (line 1124 of includes/form.inc).
The form has become outdated. Copy any unsaved work in the form below and then reload this page.
Notice: Undefined property: stdClass::$type in _node_extract_type() (line 371 of modules/node/node.module).
Notice: Undefined property: stdClass::$type in _node_extract_type() (line 371 of modules/node/node.module).
EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7633 of includes/common.inc).

I've checked it on two site builds here and i'm seeing the same thing. Let me know if you're seeing the same issue.

I haven't done too much testing of the code on D7 yet. I'll try and do more this week if I can help towards getting a 7.x release out. Much of what has been pushing me forward on this is with sites on D6 currently, so i've been testing D6 more than D7 up until now.

Happy to wait for the D7 release to happen and then backport. I normally avoid that approach as I don't like too much to stack up and then have a large batch to do at the end. I normally favour keeping up-to-date with ports so that they are reasonably aligned and up-to-date and can be tested close together. That way when i'm testing one it doesn't take too much more to test another, plus it's fresh so the use cases and test cases are clear. Happy to go with whatever works for the Drupal issue management system. Ideally on an issue-by-issue basis would work best rather than by release. You'll have probably noticed all the backporting i've done currently has tried to keep each issues patch files separate even for the backports. I'm guessing it's fairly easy to see what issues have been addressed between two releases so backporting goals can be clearly seen? Are shared feature/issue branches common or really just used on local git repos, with only the normal Drupal 7.x-1.x branches making their way to the central drupal.org git project repo?

I agree with the user password reset form being left as restricted (I assume that's the user_pass form, although i've just realised I may have assumed based on the name) so we should remove that form id from the list.

More than happy to assist with maintaining the D6 port for this module.

Hey, sorry, seems I took so long to finish typing my previous reply I ended submitting it after your followup comment.

Regarding the TRUE and FALSE values in the database, my understanding is that the variables table stores the data with the serialize() function, and boolean values are denoted by the 'b' with either a 1 (TRUE) or 0 (FALSE) value and are returned to normal TRUE/FALSE boolean values when unserialized. Therefore we should be dealing with TRUE/FALSE values rather than matching 0's and 1's in this particular instance.

Apologies if this issue has bloated out of control, probably due to my verbose comments. Feel free to tell me to keep them shorter if appropriate! :)

Re comment 25: Checked (which I should have done earlier!) and confirmed, you are indeed correct.

I've updated patch 23 with the following items: (updated patch is attached which applies cleanly on the current 7.x-1.x branch)

1. I prefer the as you put it 'truthiness' approach. I think it's cleaner and easier to read.
Using the other approach of the identical comparison operator (===) to test for equality and also check whether they are of the same type, with a boolean on the left it will convert the 1 on the other side to a boolean as well, and therefore I believe should achieve exactly the same thing. It is a personal preference, I typically favour the clearer and shorter code where possible.

I've adjusted the code to the truthiness approach, which is inline with Baris's original code, and ensured the logic is correct. (I hope!)

2. When we clear out the form, i've also cleared out the #after_build handler. We don't want other things trying to alter the form after we have cleared it out. I think this was partially the cause of the additional errors we were seeing.

3. The 'The form has become outdated' error was to do with the way the Form API works. I've adjusted things so that we retain the key FAPI build/token items. I've tested and things play nicely now. If you were editing something while Read Only mode was activated, you can successfully submit it once read-only mode is disabled. (So long as the form_build_id/token is retained on the server side, and I guess there's a timeout of some kind for the form token though I haven't checked but so long as that hasn't been reached, then forms should be accepted when returning from read-only mode.)
Another note: There may be an argument in the future for some not wanting this to happen, like if you've done a major upgrade/content-layout change on your staging server while in read-only mode, then data layout might have changed when returning from Read-only and you'd want users to edit from a fresh copy. That's a specific case and we'll cover it later should anyone request it. I suspect the simple solution to this scenario is to flush the form cache thereby invalidating any existing forms that might be out there.

4. I feel that we need to show at least something in the form when we clear it out. The status message is above where the form content would normally appear, and possibly on some sites in another place altogether, so it might not be immediately obvious when the user gets a blank/empty area. I've added a message in place of the form content. This does mean that we have two warnings (or more if there's more than one form on the page) but I think this is okay, it makes it clear to the user in the status messages area what's going on, and the form content gets a warning about the form not being available at this time.

An alternative I thought about was to show all elements but make them all disabled. However I wonder whether we could reliably parse the form and update them all to be disabled. For the time being I figure just stick with clearing out the form.

5. I've changed things slightly with regards the double error/warning message. I was only seeing the double message when I have a form open, the site enters RO mode, and then I attempt to submit the form. When that happened I got something like:

• Warning: Test site is currently in maintenance. During this maintenance it is not possible to add or edit content (like comments and pages).
• Error: Data not saved. Test site is currently in maintenance. During maintenance it is not possible to change content. Please make a note of your changes and try again later.

I've adjusted things and I now only get the Error 'Data not saved' type message, there's no warning when that occurs in that scenario.
Could you test this patch and see if you are still getting double messages at other times too? If so, could you detail exact steps to reproduce?

Summary
I've changed a fair few things in a few places to do all of the above, adjusted some of the text, moved the messages into a fieldset as the maintenance system form was getting a bit out of control otherwise. Perhaps if these are suitable, and initial testing looks good, we could get this patch applied before we start providing any further patches towards this issue?

Hopefully we are getting close to ironing out the issues and errors with this feature. If we can test it over the next couple of days and confirm that we have closed any outstanding issues and errors we find during testing, and also show confidence we haven't introduced any other bugs along the way, maybe we'll be able to close this feature as fully tested and get a new release tagged up in a few days.

Daniel, I think I covered all of your items in your last comment and fingers crossed everything is now working correctly.

Baris, I hope we haven't overwhelmed you too much with the amount of sudden activity on this module this week :)

Such are the ways of weakly typed languages. I typically refer to the language reference manual when it comes to these sorts of edge cases as I wouldn't assume all languages operate the same.

With regard your examples:

php > var_dump((0 == 'TRUE'));
bool(true)
php > var_dump((1 == 'TRUE'));
bool(false)
php > var_dump((int)'TRUE');
int(0)

The first two are loose comparisons between a string and a number. (A useful type comparison table is available http://php.net/manual/en/types.comparisons.php)

The 'Comparison with Various Types' table at http://php.net/manual/en/language.operators.comparison.php is helpful too.

PHP defines its approach as: If you compare a number with a string or the comparison involves numerical strings, then each string is converted to a number and the comparison performed numerically.

Then, for string conversion to a number, this is defined here http://php.net/manual/en/language.types.string.php#language.types.string...
as: If the string starts with valid numeric data, this will be the value used. Otherwise, the value will be 0 (zero).

So for your examples, based on the language definition I would expect:

1. 'TRUE' to be converted to 0 as it's just a string, giving the comparison 0 == 0, giving a result of boolean TRUE.
2. 'TRUE' to be converted to 0 as it's just a string, giving the comparison 1 == 0, giving a result of boolean FALSE.
3. 'TRUE' to be converted to 0 as it's just a string, which becomes (int)0, giving a result of integer 0.

Perhaps not what you were hoping to hear, but hope that helps in some way.

Backported.