I am opening this issue in order to be able to submit a patch that fixes the following issues. I'm sorry for submitting a single patch for all those issues, but it turns out that just a few changes allowed all those improvements.

1) Do not overwrite $_REQUEST variable
This simply can cause problems to other modules using it, therefore it is better to introduce a new variable rather than overwriting it.
#1496310: Do not overwrite $_REQUEST variable

2) Allow default values set through the URL to actually be modified by users during form editing.
Before this patch users were able to edit the default values, but those values were not saved, which was quite a serious issue.
#1473856: Overzealous prepopulation
#1309934: After prepoulating fields (new) content can not be saved if it was edited before

3) Allow to prepopulate 'hidden' and 'value' field types.
I see no reasons to prevent setting default values those field types.
#1275274: Allow use with #type = 'hidden'

3) Add administration options (and a related permission) to lock default values and overwrite the ones already present for the field.
#926094: Able to lock the field after populating?
#672400: Working around field defaults?

4) Remove prepopulate.install and replace it with hook_module_implements_alter implementation in prepopulate.module.
I am not sure this is really needed, but it is surely better implemented as a hook.

5) Add usage information in the module UI help page.
Just to make the usage more clear to new users.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Norberto Ostallo’s picture

Status: Active » Needs review
FileSize
15.79 KB

Here it is the patch.

jbrauer’s picture

Status: Needs review » Needs work

Thanks for these suggestions.

I think this needs to be broken up into smaller issues. In part some of this looks like a good start and could be included sooner than later. Others like the hidden field values are more significant and will have several other dependencies. Marking as needs work but for most items there is a separate issue where a specific patch would be appreciated.

Norberto Ostallo’s picture

I'm glad to hear you find this interesting and I will try to break this up into smaller issues, providing a patch for each of them.
In this way it should be easier to check and validate each of them.

Norberto Ostallo’s picture

Status: Needs work » Needs review
FileSize
4.06 KB

I have reduced the scope of this patch, opening other issues for minor tasks.
The following are the major changes to the module.

  1. The use of a #process function in stead of the #after_build. This is essential in order to allow users to change default values set through the URL and fixes the usability issue described in #1309934: After prepoulating fields (new) content can not be saved if it was edited before. I think it is quite a bad behavior to save a content with a value different from the one a user has edited in a form.
  2. The use of #default_value property in stead of #value, which is the correct way of setting a default value in a form element. This allows other module to perform their actions on the default value and allows users to change it before submission.
  3. Thanks to the changes in 1. and 2., I have been able to implement an administration page, where admins can set "overwrite" and "lock" options on fields. These fixes #926094: Able to lock the field after populating?.
Norberto Ostallo’s picture

I'm sorry, patch in #4 was broken because it did not include a new file.
This patch should work and I have also added a first way to allow admins to restrict prepopulation on selected forms.

jbrauer’s picture

Status: Needs review » Needs work

1. cannot be done because it does not adequately address security concerns. Other modules could act after Prepopulate and change access to the field so the only place this can be done securely is in #after_build. Any further work on that should be in #1309934: After prepoulating fields (new) content can not be saved if it was edited before.

2. The #default_value does not work in all cases which lead to having to use both #default_value and #value which is not as much an issue because item 1 precludes editing fields that have been pre-populated.

The only way I've been able to see around these issues which are in #1309934: After prepoulating fields (new) content can not be saved if it was edited before would be to implement an administration system where site administrators can choose what forms and ideally can be prepopulated. Absent this level of administrative control site would be open to CSRF.

These issues have already been fixed once and can't be casually made insecure again. See http://drupal.org/node/1182968 for related information.

thekevinday’s picture

I was thinking entityreference_prepopulate has a better approach all around.

In particular, entityreference_prepopulate makes the arguments cleaner and easier to figure out.
The downside I think would be that each field type would have to have support written for it, so custom modules would each need their own prepopulate handlers.

All of 1->5 are solved by the strategy used by entityreference_prepopulate.

The only problem is its explicitly for entityreference.
Perhaps refactoring and using a more generalized version of it would be a better start than the current 7.x version of prepopulate, which has not seen any changes in a year (and never really was written/released for D7).

Edit:
see also: #1612548: Prepopulate integration

Robin Millette’s picture

Issue summary: View changes

Updated issue summary. Linked to the "Do not overwrite $_REQUEST variable" issue.

jbrauer’s picture

Issue summary: View changes
Status: Needs work » Closed (outdated)

These issues have evolved in other directions leaving this to be outdated.

manoloka’s picture

jbrauer ... for what you say in #6 patch in 5# should not be used as it would make the module unsafe, is that right?

Thanks

manoloka’s picture

Norberto Ostallo if 1. and 2. from patch in #5 are not safe (as says jbrauer in 6#).

Would it be possible for you to make a patch to implement the admin and lock function?

That'd be a great addition to the module

Thanks for your work
Manolo