Data Dictionary

Hi dwieeb,

This sounds a lot like an existing project: http://drupal.org/project/wysiwyg_template Have you tried that? It would be useful to explain differences in the project description.

* README.txt ist missing
* Git release branch is missing, see http://drupal.org/node/1015226
* "Implementation of hook_menu()" should be "Implements hook_menu().", see http://drupal.org/node/1354#hookimpl
* Indentation should be always 2 spaces, not 3, see http://drupal.org/node/318#indenting
* you should run the coder module to check your code style: http://drupal.org/project/coder

* do not include LICENSE.txt, this added by drupal.org packaging automatically.
* "foreach ( $variables as $key => $value )" there should be no spaces after '(' and before ')'. Also for the if statements in your code.
* admin_main.js: indentation should also be 2 spaces, also for the other javascript files

Not sure about all the markup in the form, if it should be in a theme function or a template ... leaving this for another reviewer to check.

Adding section headers. Adding deps and diffs section

I agree, the form should use theme('table') to generate the markup. Otherwise admin themes won't be able to control the table display.

- Use theme('table") for the admin form theme_table
- Move CSS for the admin into a file. That way themers can override them if need be.
- Storing all the key/value pairs in one large variable in the variables table seems like it may cause some issues later on. Especially if you reach hundreds or thousands of keys or lots of very long keys (you may reach the mysql packet size limit on a larger dictionaries). I would suggest creating a table of key/value pairs along with cache system integration. May be slightly slower for smaller dictionaries but will save you on larger ones.

As a side note, had you considered using the Token system along with the Token Filter module?

Tables in forms are messy. I've done them similar to how dwieeb did them here, putting the table markup in #markup, #prefix, and #suffix. I've never seen any complaints about this approach. It's is relatively easy to read and can be changed with a form alter function, which themes can do now in D7.

Core does something different in places like the user permissions table, but I haven't been able to figure out how that gets from the abstract form object to the eventual HTML table output. We don't seem to have any community initiative to document and advocate the approach core takes, so I think it's okay if modules like this take the more obvious approach.

I do think you should remove the inline style in the form markup, and apply style with classes and separate CSS files instead. Separating structure and design makes it easier to alter both independently. Also, unless you have a good reason to use double quotes, you should wrap markup in single quotes. Single quotes make double quotes easier to read (since you don't have to escape them with \) and also executes slightly faster.

Everything I've mentioned changing here is pretty minor, so I'm moving this to RTBC.

#10 was cross-posted with #9. I didn't intend to jump from needs work to RTBC. So needs review is good.

• "'description' => t('The variable key used to find this variable.'),": do not translate DB descriptions, this will just generate overhead for translators.
• "'not null' => true,": code style, should be "'not null' => TRUE,"
• "catch(Exception $e) {" should be "catch (Exception $e) {", coder did not detect this?

Otherwise I think this is nearly ready.

There is a security vulnerability in datadictionary_node_view() on line 461. You are embedding $value without sanitizing the input. I was able to exploit that by using a value of <script>alert('XSS');</script> in a body field that was set to have a plain text filter.

You must respect the text format setting of the body field, currently you are ignoring it. I don't think that hook_node_view() is ideal because it looks like the body field content is already rendered there. Maybe another hook might be an option, because then you could inject your values before text formats are applied.

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/datadictionary.module:
   +97: [minor] Use an indent of 2 spaces, with no tabs
   +98: [minor] Use an indent of 2 spaces, with no tabs
   +99: [minor] Use an indent of 2 spaces, with no tabs
   +146: [minor] There should be no trailing spaces
   +319: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +333: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +340: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +360: [minor] There should be no trailing spaces
   +362: [minor] Use an indent of 2 spaces, with no tabs
   +364: [minor] Use an indent of 2 spaces, with no tabs
   +411: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
  
  sites/all/modules/pareview_temp/test_candidate/datadictionary.install:
   +36: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
   +36: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
   +51: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
   +51: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
  
  Status Messages:
   Coder found 1 projects, 7 files, 2 critical warnings, 2 normal warnings, 11 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

tiny_mce_popup.js: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

• regarding the library: please open issue in the webmaster's issue queue to get clearance for the library. Or ask the footnotes maintainers where they got it, I want to see a reference here.
• For the substitution hook: maybe hook_field_formatter_prepare_view() works or a similar field API hook. You could also improve your module that it works with any field, not just the body field.
• "if (!isset($node->content['body']))": all if structures should be wrapped in "{}", see http://drupal.org/node/318#controlstruct

Hm, I really think you should implement hook_filter_info() and others that belong to that. No, that does not filter on save, but on output. I just reviewed Multicolumn http://drupal.org/node/1181988/ which implements something similar very clearly.

I hadn't looked into Token Filter, but it looks like in order to add custom tokens to the Token system, the user must implement some semi-complicated hook functions, unless I'm misinterpreting. Data Dictionary provides a simple interface for site administrators to manage their variables as well as buttons for users to insert the variables into their wysiwyg editor.

Just wanted to point out that this is what the http://drupal.org/project/token_custom module is for as well. It may be worth pointing on on this project's page that Token Filter + Custom Tokens would be an alternative to this module.

It was my first thought as well that your module's idea would be better implemented as some kind of add-on for tokens. Not only would that seem better to me in the sense of global module consistency, also does it feel uncomfortable to have to many proprietary token/placeholder styles. The latter is just a nebulous personal feeling of course, but IMO you should really consider hooking into the token stuff.

Besides that, I really like your idea and without much thinking I could find a lot of points in my own sites where such a function would make big sense.

dwieeb, what are your intentions? Should this remain in a "needs review" status, or, do you intend to postpone this until you form some plan with the Custom Tokens maintainers?

There have not been replies in the past months.

changing to dev brance