name: sitevars
version: 7.x-1.0

sandbox: https://drupal.org/sandbox/kris-o3/1833646

description:

Edit a key-value store, accessible from Configuration > System > Site variables.

Specify keys as a comma-separated list, then a value for each key. This is stored in {variable} by the name sitevars_keys. Then specify a value for each key, which is stored with the prefix sitevar_ (this establishes a namespace distinct from existing variables). Empty strings will not be stored in the database; deletion is automatic.

The function sitevars_get() is provided for convenient retrieval of values, without the need to specify the sitevar_ prefix when calling variable_get(). Pass it a string to retrieve one variable, an array of strings to retrieve several; call it with no arguments to obtain your entire key-value store.

Comments

wolvern’s picture

wolvern commented

Hello kris-o3,

Firstly, you should sort out the line breaks in your files, the autoreview script sees most of the code in a single line: http://ventral.org/pareview/httpgitdrupalorgsandboxkris-o31833646git

After that, note that you are using a somewhat different coding standard from what is expected in Drupal code: http://drupal.org/coding-standards.

You've completely forgotten to include any inline comments for your code. Check out here: http://drupal.org/node/161085.

Although, I think, the main question that should be discussed is the need for such a module in the first place. I can hardly imagine a site builder who would need a variable to be used in code, but who wouldn't know how (or would choose not) to set one using variable_set(). But I could be missing something. Could you describe a situation where this module would come in handy?

ankitchauhan’s picture

ankitchauhan commented
Status: Needs review » Needs work
kris-o3’s picture

kris-o3 commented

to be fair, i installed and ran the Coder module extensively prior to adding/committing my module for review, so beyond linebreaks being LF (os x/linux) instead of CRLF (windows) and the lack of comments explaining the functions (of which there are only a few)... what else did i do "wrong?"

per the more important question as to what use case this module serves... while it's true that there are individual modules to manage, say, api keys or site id's for google analytics, another for mailchimp, still another for salesforce, etc. etc. ad nauseum, i came up empty looking for a module that serves my own needs for a simple interface to define some variables i could call upon in my custom themes/templates.

i had recently done something similar using a content type that contained a field collection of zero to many name/value pairs, and while this worked, felt just awful. storing configuration in content. ugly.

so i made this module.

it's true that a developer could, in their template.php file, use variable_get and variable_set as needed, but if a nontechnical person ever needed to update say, again, an api key for some 3rd party service, that person could do so in a simple form rather than digging into code or opening a trouble ticket requiring billable time w/ a developer.

so there's the use case.

i'm actually still working on this, so if the linefeeds are a Major issue i can convert to CRLF and i will indeed comment my code so that anyone who chooses to extend this module will know what's going on... in addition i'm tweaking things so it stores all key/value pairs in a serialized array in a single variable, for performance and efficiency sake.

smiletrl’s picture

smiletrl commented

Hi kris-o3,
Agreed with saulelis, code in this module is using a different standards. And I think we could add some explaination for each function, like 

/**
 * Implements hook_simplenews_unsubscribe_user().
 */
function crm_core_news_simplenews_unsubscribe_user($subscriber, $subscription) {
}

This part 'page callback' => "sitevars_settings_callback",, if all the page callback does is to render a form. I think using drupal_get_form as page callback and form id as 'page argument' would be a better choice.

As for the use case of this module, I agree that storing configuration in content is not a good practice. Thing is, what does u what to do with ' many name/value pairs'? The first thing coming to my mind is to use a select list field in content type. And if you want to collect data from user, you could use a form. For nontechnical person, if using variable_get and variable_set is difficult, should be using sitevars_get() much easier? Anyway, only a suggestion:)

kris-o3’s picture

kris-o3 commented

"I think using drupal_get_form as page callback and form id as 'page argument' would be a better choice"

tried that, didn't work. i was going by what i was taught in an Acquia-approved module development course... while possibly unnecessary is it really a 'problem' to do it this way?

again to the more important question of use-case...

would you not agree that, at times, a site administrator and front-end theme developer are Often not the same person?

the idea is simply to provide an interface to store/retrieve some variables that are referenced in the front-end theme using variable_get or the provided utility function sitevars_get... once the site is deployed/launched, someone other than the front-end theme developer would be able to easily update any of the variables, or add a new one and ask their front-end dev to display it in the theme somewhere.

i imagine this is why things like email and site slogan exist in the "Site information" form, yes? the idea here is precisely the same.

smiletrl’s picture

smiletrl commented

Hi,
If you put it like email and site slogan, then I'm okay with that:)
For page callback, it's just a personal practice. It's not a 'problem' to do this anyway. But, could u paste your code for this part? I'm just curious that this doesn't work for you.
Also, there's function of preg_replace in .module. Use preg_replace_callback() instead. See https://drupal.org/node/750148

kris-o3’s picture

kris-o3 commented

per that post, i'm not using the /e flag, so preg_replace is fine.

kris-o3’s picture

kris-o3 commented

now version 7.x-1.1

  • key-value store now in single, simple associative array
  • many user-interface improvements
  • added comments explaining all functions
  • additional outputs from sitevars_get( ) function
kris-o3’s picture

kris-o3 commented

now version 7.x-1.2

added ability to filter list of variables by a prefix.

kris-o3’s picture

kris-o3 commented
Status: Needs work » Needs review

changing status from 'needs work' to 'needs review'

stborchert’s picture

stborchert
he/him
Primary language German
commented
Status: Needs review » Needs work

Back to "needs work" since there are several task to do:

Master Branch
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Major coding standards / best practice issues
An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

You can find the results of the automated report at http://ventral.org/pareview/httpgitdrupalorgsandboxkris-o31833646git.

Code
  • You are using Windows (?) line endings. Files should be formatted with \n as the line ending (Unix line endings), not \r\n (Windows line endings).
  • The dependency to "system" is not needed.
  • Remove the version tag from your .info file since it is generated by Drupal.
  • "package" is not needed since "Other" is the default package for modules.
kris-o3’s picture

kris-o3 commented

"You should really be working in a version specific branch."
- even for a project that hasn't yet made it past its 1st review to become a full-fledged project?

"You are using Windows (?) line endings"
- that's funny because i got a complaint on a different project that i was using Unix line endings and to please use CRLF.

as for the last 3 bullets in Code, noted and will resolve.

kris-o3’s picture

kris-o3 commented
Status: Needs work » Needs review

http://git.drupal.org/sandbox/kris-o3/1833646.git
http://ventral.org/pareview/httpgitdrupalorgsandboxkris-o31833646git

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

(now completely blank. clean report.)

kris-o3’s picture

kris-o3 commented

re-committed after a final review by both Coder and PAReview.sh

ain’s picture

ain commented
Status: Needs review » Reviewed & tested by the community

Interesting idea for a module.

Code review

Automatic

No errors, all clear.

Manual

Everything seems in order and check_plain() is used to evaluate user data entry. Uninstall cleans up nicely.

Testing

Testing was a success, module does everything that it says to do.

Recommendations

  1. CHANGELOG formatting could use a common standard of: version - dd.mm.yyyy \n dashline \n bulleted list

NB! Make sure to review 3 other modules and tag this application with PAReview: review bonus in order to get it resolved for good. Check the Review bonus for more info. Otherwise I think it's RTBC.

kris-o3’s picture

kris-o3 commented

i might have guessed there was a standard for changelog, my mistake honestly. will edit asap.

kris-o3’s picture

kris-o3 commented

changelog updated.

jthorson’s picture

jthorson commented

... while possibly unnecessary is it really a 'problem' to do it this way?

One of the goals of this review process is to help develop familiarity with and proper use of the Drupal APIs; so while the code may work, reviewers are always going to point out the recommended or best practice approach. Encouraging 'one way' to do things helps ensure consistency between contrib modules, which is a benefit to end users.

... even for a project that hasn't yet made it past its 1st review to become a full-fledged project?

Similar to the 'proper use of Drupal APIs' comment above, we like to use this review process to demonstrate familiarity with the branch & tag naming patterns.

- that's funny because i got a complaint on a different project that i was using Unix line endings and to please use CRLF.

I'd be interested where you got this complaint ... the standard is definately Unix-style line endings in the drupal.org repositories.

jthorson’s picture

jthorson commented

Rather than using $_SESSION['sitevars_filter'], I'd suggest storing the preference in the $user object (as described in http://www.braahm.be/node/7).

t(": <em>@filter</em>", array('@filter' => $filter)):
As per the documentation at http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/format_..., You can use %filter here instead of @filter, which will include the <em> tags for you.

t("@spacewith prefix: <em>@filter</em>",
I'm curious about the @space variable here ... simply including a space at the beginning of the string would be just as effective, and would be more straight forward for translators.

t("Are you sure you want to delete @plural?", array('@plural' => (count($names) != 1 ? "these variables" : "this variable"))),
I think the context of the 'this/these variables' portion of this string is lost for translators ... use format_plural() instead of t() here. See http://api.drupal.org/api/drupal/includes!common.inc/function/format_plu...

t("@plural !list deleted.",
Same concern (providing context to translators). Use format_plural().

sitevars_form
I'd recommend adding a sitevars_add_validate() routine (and the equivalent sitevars_filter_validate()), to alert the user when they've supplied non-allowed characters, rather than simply stripping them from the input.

jthorson’s picture

jthorson commented
Status: Reviewed & tested by the community » Fixed

None of the above are showstoppers, so ...

Thanks for your contribution, kris-o3!

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.

kris-o3’s picture

kris-o3 commented

huh! format_plural... there's so much to drupal, reading lists of functions is like reading the dictionary :)
will look into that for a future update...

definitely also didn't know about storing additional 'temporary' info in $user... that's pretty cool.

the form description Does indicate which characters are considered valid for a variable name...
but yes, better UX would be to both indicate as well as validate.

hooray! it's a project! thanks, all. i did actually learn a lot during the review process.

kris-o3’s picture

kris-o3 commented

hmm, i followed the instructions to tag 7.x-1.x as 1.0 and created a release, but still not showing up in Downloads at /project/sitevars

git status
# On branch 7.x-1.x
nothing to commit, working directory clean

git checkout 7.x-1.x
Already on '7.x-1.x'

git tag 7.x-1.0
fatal: tag '7.x-1.0' already exists

git push origin tag 7.x-1.0
kris-o3@git.drupal.org's password:
Everything up-to-date

kris-o3’s picture

kris-o3 commented

.zip finally now appearing. SWEET.

jthorson’s picture

jthorson commented

Publishing of releases takes up to 17 minutes after 'go' ... and we've had some instances of the publishing script hanging, and having to be restarted in the past few weeks.

jthorson’s picture

jthorson commented

Another implication to using $_SESSION for storage ... Drupal 7 will disable anonymous page caching if a session exists, which can cause performance issues on large/busy sites.

kris-o3’s picture

kris-o3 commented

i guess drupal core does it entirely wrong, huh...

line 139 of modules/node/node.admin.inc, in function node_filter_form

$session = isset($_SESSION['node_overview_filter']) ? $_SESSION['node_overview_filter'] : array();

and line 225, in function node_filter_form_submit

$_SESSION['node_overview_filter'][] = array($filter, $form_state['values'][$filter]);

this is actually where i got the idea to do it this way...
from the CORE NODE MODULE /admin/content form.

jthorson’s picture

jthorson commented

In the case you reference, the pages are on the /admin/% path. By their nature, these paths are not made available to anonymous users, and so the implication of 'disabling page caching for anonymous users' does not apply.

So whether it becomes a concern for sitevars depends on the use case ... as long as nothing is written to $_SESSION for the 'anonymous' role, then this can be avoided. I just wanted to point out the potential for an issue when using $_SESSION, especially given that alternatives such as storage within $user->data exist.

kris-o3’s picture

kris-o3 commented

that is correct. it is only used to persist what is typed in 'filter by prefix' on the administration/config form for sitevars.
as seen here: http://drupal.org/files/project-images/sitevars.png

Status: Fixed » Closed (fixed)

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