This module enables Drupal to remember entered values from selected fields upon new node submission, stores them in the database and then pre-fills appropriate fields on the next node-add form. This is useful when a user needs to enter many nodes at a time and a few of the values are repeating -

I've added a support for a handful of field types so far. I want to polish them out before adding more. I think this is a better approach than having a ton of fields to deal with from the beginning.

I got a 6.x version in git and I'm already halfway done with a 7.x version.

This is my first Drupal module, I hope to receive some feedback and work my way to a full project.

Project page:
http://drupal.org/sandbox/saulelis/1627616

Git:
git clone --recursive --branch 6.x-1.x http://git.drupal.org/sandbox/saulelis/1627616.git remember_values

My reviews:
http://drupal.org/node/1612022#comment-6681314
http://drupal.org/node/1833682#comment-6711254
http://drupal.org/node/1777990#comment-6679174

CommentFileSizeAuthor
#8 drupalcs-result.txt2.14 KBklausi

Comments

wolvern’s picture

wolvern commented
Andreas Radloff’s picture

Andreas Radloff commented

Great work with the coding standards, automatic review found nothing to complain about. So, lets get on with the manual complaining! ;-)

Here's a similar module comparison:
This module - Sets defaults per browser (cookie) and clears them after cookie timeout.
http://drupal.org/project/elementdefaults - Sets form defaults through url / link
http://drupal.org/project/form_save_state - Sets form defaults per browser and clears them on submit. Does all the heavy work with Javascript in the frontend and saves to LocalStorage.
http://drupal.org/project/autosave - saves defaults per user server side and clears them on submit.

What you want to do is different, not clearing the values but rather setting them on form submit.

I only had to change two lines in the module form_save_state to make it work like yours does, with the exception that you let the user set configuration per form element rather than enable on entire forms. Have you considered contributing a setting to any of the above modules to make it work like you want to?

I don't think storing the values in a cookie is the right approach, first of all cookies can only be 4kb, which might not be enough. Secondly cookies are sent by the browser to the server with EVERY request, slowing down the connection and putting load on the server. For this reason, you should only store an id in the cookie and keep the values on the server, using the cookie id to retrieve them.

Any thoughts on this?

wolvern’s picture

wolvern commented

Hi Andreas,

thanks for your input.

I've looked through the modules you mentioned. Form save state and Autosave are similar between them, but not so much to mine. They both depend on a respectable amount of javascript code, which is totally unnecessary for my purposes. For that reason alone I wouldn't consider them seriously as possible candidates for merger.

Form element defaults is much more alike and indeed we share a similar need to change the node edit form via hook_form_alter. So maybe we could share the functions for filling in the default values in the forms, but I think that's where similarities end. I doesn't make sense for my module to pass values via URL, I need to store them on the server. Also, the purposes and possible application situations of our modules are quite different. I've contacted the author to see what he thinks.

I don't think storing the values in a cookie is the right approach

I totally agree. It was always at the back of my mind, I just never got around to it so far. I'll do that now.
UPDATE: I've done away with the cookies completely.

wolvern’s picture

wolvern commented

Here's a similar module comparison:
This module - Sets defaults per browser (cookie) and clears them after cookie timeout.

Do you mind confirming which module you meant here? You forgot ton paste in an URL :)

wolvern’s picture

wolvern commented
Issue summary: View changes

just a couple typos

Andreas Radloff’s picture

Andreas Radloff commented

Oh, I meant your module. Sorry for being unclear. :)

Andreas Radloff’s picture

Andreas Radloff commented
Issue summary: View changes

I had to change description, because the project changed a bit

wolvern’s picture

wolvern commented
Issue tags: +PAreview: review bonus

Added a list of my reviews

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

Really like the sound of this module!

Ran the code through ventral.org & it pulled up some issues: http://ventral.org/pareview/httpgitdrupalorgsandboxsaulelis1627616git

Just had a read through the code manually & found a few things:

1. In a couple of areas (mainly within remember_values_form_node_type_form_alter) you're setting the description to be nothing, is this intentional or could you just remove it?

2. There are quite a few if statements which are really hard to read & figure out what's going on, line 275 of remember_values.module for example spans quite wide, could the db queries perhaps be separated into variables & then process them?

3. Within remember_values_node_type_form_submit do the indexes need to be set for the $values array, perhaps $values[] might be better. You also access the $values array within the foreach loop but there's no guarantee it would contain any values. There's also a call to variable_del & then variable_set to the same variable, variable_set should be all you need as it will overwrite the previous value.

4. In the settings form: "Enable remembering form values in all content types." doesn't read that well for me, perhaps "Remember form values for all content types" might be a bit better

5. Small spelling mistake: ~line 365 // Get CCK fieds and add to the form.

Other than that I can't see any major issues.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue tags: -PAreview: review bonus +PAreview: security
StatusFileSize
new drupalcs-result.txt2.14 KB

Review of the 6.x-1.x branch:

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  1. all the modules that Andreas Radloff mentioned should be listed on your project page and the differences to them, so that it easier for users to choose the right module for their needs.
  2. remember_values_install(): why do you need to manipulate the module weight? Please add a comment.
  3. "define('VALID_WIDGETS'": all constants defined by your module need to be prefixed with your module's name to avoid name collisions with others.
  4. "db_query("SELECT filepath FROM files WHERE fid=%d"": database tables need to be enclosed with "{}" in db_query(). See http://api.drupal.org/api/drupal/includes!database.inc/group/database/6
  5. notice: Undefined index: #node in remember_values.module on line 335. Please enable PHP notices in your development environment.
  6. notice: Trying to get property of non-object in remember_values.module on line 335.
  7. remember_values_form_node_type_form_alter(): this is vulnerable to XSS exploits. If I have a vocabulary with the name <script>alert('XSS');</script> I get a nasty javascript popup on the node type configuration page. You need to sanitize user provided input before printing into #title. Please read http://drupal.org/node/28984 again.
  8. _remember_values_write_db(): use drupal_write_record() instead of direct insert/update queries and you get schema validation for free.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

wolvern’s picture

wolvern commented
Status: Needs work » Needs review

Guys, thanks for your reviews. I've addressed all the issues you've mentioned.

klausi, just one remark on your 1. - I mentioned Form element defaults in my project page (although I kind of doubt there is really need for that...), but others from Andreas's post are just too dissimilar from user's point of view to be included. They are just made for completely different purposes. What you think?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

Did another manual review and I think this is RTBC now.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

no objections for more than a week, so ...

Thanks for your contribution, saulelis!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

wolvern’s picture

wolvern commented

Wonderful! Thanks everyone for helping out with the process!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added my reviews