[D7] Field Concat Display

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxjschoen12260057git

I'm a robot and this is an automated message from Project Applications Scraper.

I really like the idea of this module, found myself needing something like it previously.

After setting up a new cat field I get this error message:

Notice: Undefined offset: 1 in _field_concat_display_process_selected_field() (line 86 of /var/aegir/platforms/dev_727/sites/example.com/modules/field_concat_display/field_concat_display.module).

And the field doesn't appear on the node (see screenshot) however I can see it as a field under Manage Fields of the content type.

Asking about clearing the cache is never a silly question... solves so many problems!

I tested it again on a fresh install of Drupal with the same code I used yesterday and it didn't throw any errors. So, it works as you described!

But, I did find a couple more things that I'm sure you will be able to reproduce:

1- When you save a node leaving one of the source fields blank it gives this error:

Notice: Undefined offset: 1 in _field_concat_display_process_selected_field() (line 86 of /var/aegir/platforms/dev_727/sites/example.com/modules/field_concat_display/field_concat_display.module).

2- When on the Field Concat Display tab (admin/structure/types/manage/page/concat_fields) you can add an empty concat field and then it throws more errors. See the attached screenshot. Reflecting on my first test, I think this is how I ended up with those original errors because I didn't realize that I accidentally added second concat field with out any source fields.

Other than those things, It worked great. I added two concat fields to the same content type and as long as I had something in every source field it worked as you described.

Those are working for me too now.

I found one more thing: When you disable the module and then uninstall it I don't think it's clearing the variables. After I uninstalled it completely I could enable it a new and the concat field was available again on the content type but I hadn't yet configured anything.

Other than that I think it's working well. I'm not sure what happens from here if all is well.

hi,

After creating new node I got following errors

Strict warning: Only variables should be passed by reference in _field_concat_display_process_selected_field() (line 82 of D:\xampp\htdocs\d7\sites\all\modules\field_concat_display\field_concat_display.module).
Notice: Undefined offset: 1 in _field_concat_display_process_selected_field() (line 87 of D:\xampp\htdocs\d7\sites\all\modules\field_concat_display\field_concat_display.module).
Strict warning: Only variables should be passed by reference in _field_concat_display_process_selected_field() (line 82 of D:\xampp\htdocs\d7\sites\all\modules\field_concat_display\field_concat_display.module).
Strict warning: Only variables should be passed by reference in _field_concat_display_process_selected_field() (line 82 of D:\xampp\htdocs\d7\sites\all\modules\field_concat_display\field_concat_display.module).
Notice: Undefined offset: 1 in _field_concat_display_process_selected_field() (line 87 of D:\xampp\htdocs\d7\sites\all\modules\field_concat_display\field_concat_display.module).
Strict warning: Only variables should be passed by reference in _field_concat_display_process_selected_field() (line 82 of D:\xampp\htdocs\d7\sites\all\modules\field_concat_display\field_concat_display.module).

Project path given "git clone --branch 7.x-1.1 http://git.drupal.org/sandbox/jschoen1/2260057.git" seems to be wrong!!

Hi, I think this is a cool idea for a module, and definitely simplifies something that often has to be done in code. That said, here are my comments:

1. I'm also getting the "Strict" warnings. I believe this is a PHP version issue, and you can resolve it by splitting that line (82) into 2, first getting the render array into a variable, and then afterwards passing it to render().
2. It's a shame that it's not possible to add other "extra fields" to the concatenated fields, for example Display Suite's title field. That is probably not easily added, but would be cool.
3. I notice that a space is automatically added after the prefix and before the suffix. Personally I would prefer if no space was added. The user can add a space if they want, but maybe in some cases they want the prefix/suffix to be flush with the value of the field.
4. I think the stripping of all tags except links is somewhat arbitrary. I would consider either mentioning this in the documentation/help, or removing the strip_tags. FYI if you want to get the value of the field without the extra divs that get added to fields, you can use field_view_value() instead of field_view_field().
5. You might consider having the view mode passed along to the _field_concat_display_process_selected_field() function, so any custom display settings per view mode will be used.

Anyhow, cool module, good luck!

P.S. For the obligatory uber-picky code review: in the .module, missing space on line 83, and extra indent on line 88.

From the updated code it looks like this will only output the first value of multi-value fields. Shouldn't it combine all the values together for multi-value fields? Or if that's intentional, there should probably be some mention of it in the docs.

# Please change git url to git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jschoen1/2260057.git field_concat_display in the project application.
# Pareview gives an error: http://pareview.sh/pareview/httpgitdrupalorgsandboxjschoen12260057git, please fix this.

Manual Review
# field_concat_display.info: Add . at the end of description
description = concatenate fields without saving in database.

# field_concat_display.module file

/**                                                                                                                                                    
 * Implements hook_menu().                                                                                                                             
 */
function field_concat_display_menu() {

  // Path to the edit form for each content type's pseudo fields.
  $items['admin/structure/types/manage/%/concat_fields'] = array(
    'title' => 'Field Concat Display',
    'type' => MENU_LOCAL_TASK,
    'page callback' => 'field_concat_display_settings_page',
    'access arguments' => array('administer field concat display'),
    'file' => 'field_concat_display_admin.page.inc',
    'weight' => 10,
  );
  ...
 }

Change this to

/**                                                                                                                                                    
 * Implements hook_menu().                                                                                                                             
 */
function field_concat_display_menu() {

  // Path to the edit form for each content type's pseudo fields.
  $items['admin/structure/types/manage/%/concat_fields'] = array(
    'title' => 'Field Concat Display',
    'type' => MENU_LOCAL_TASK,
    'page callback' => drupal_get_form',
    'page arguments' => array('field_concat_display_settings_form'), # Page arguments is added.
    'access arguments' => array('administer field concat display'), 
    'file' => 'field_concat_display.admin.inc', # This is changed 
    'weight' => 10,
  );
  ...
}

Also, it is a good practice to keep your admin functions in .admin.inc file, in your case the file field_concat_display_admin.page.inc should change to field_concat_display.admin.inc, also this has to changed in hook_menu().

So the function below is redundant, you can get rid of them, once to change your hook_menu().

/**                                                                                                                                                    
 * Admin form page callback.                                                                                                                           
 */
function field_concat_display_settings_page() {
  $form = drupal_get_form('field_concat_display_settings_form');
  return $form;
}

Hey, you didn't quite get there with the multi-value support. It still only displays the first one. Try this code as a replacement starting line 72, it should help you out:

$items = field_get_items($entity_type, $entity, $field);
foreach ($items as $item) {
  $display[] = field_view_value($entity_type, $entity, $field, $item);
}

This seems good to me!

I can confirm this module works as advertised. After testing, we have pushed this all the way into our production environment.

Thanks jschoen1.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Added PAreview link.

PAReview came up clean. I am seeing a little odd formatting and long lines, so I think this may be a false negative.

Formatting in README.txt is a little weirda tht top.

Your try/catch logic in _field_concat_display_get_own_variables is a little weird. What do you expect an
exception to be? This whole thing is really equivalent to a wildcard db_delete() followed by clearing the
variable cache. Wildcard variable deltion is frowned upon b/c you may have namespace conflicts.

field_concat_display.admin.inc is the standard for naming the include for settings forms.

variable_set/get will handle the serialization for you. No need to manually go between JSON.

I want to take a closer look at the string building in the admin forms.

Normally setting forms are handled in a different manner. See https://drupal.org/node/206761 Though, this may not apply in your case. Read it through and see.

The logic in field_concat_display_node_view_node() and _field_concat_display_process_selected_field() is a little weird.
Stripping the tags except for seems a little too specific to one case. You have a theme function, but you are also
explicitly using #markup. Seems like you need a default preprocess function for your theme function.

You don't need to t() strings that are simply a replacement.

field_concat_display_field_extra_fields(), arg() should be avoided. Same as for function _field_concat_display_get_node_type() {
(). See if there is a better option here.

Use drupal_html_class() istead of your own logic for class names.

(*) field_concat_display_delete_pseudo_field_callback() should use the arguments that get passed to it instead of pulling
them in with arg().

I think you can simplify some of the validation by using the default element level functions. See element_validate_number and friends.

Rather than validating against empty(), make the form element required.

Double check the regex in field_concat_display_new_field_validate for field names against what core uses.

When using variable replacement in double-quotes strings, it can help to use the wrapping {}; this makes the code a little easier to read.

The admin form is a little counterintuitive. Maybe some better labels?

(*) The concatenation is XSS vulnerable. I created two text fields, and set the label and prefix for all to be
<script>alert('XSS');</script>. I got an alert for all four. You need to check_plain() these before the theme function.

The starred items are blockers. Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This is a really neat idea and looks very useful, however the approach has quite a few conceptual problems.

function field_concat_display_node_view($node, $view_mode, $langcode) {

It only works on nodes! In D7, any module that advertises itself as working with fields should work on all entities.

> and stores this pseudo data in the variable table

That means the settings aren't cleanly exportable.

function field_concat_display_field_extra_fields() {

  $raw_node_type = arg(4);

How do you know where you're being invoked? Also, this hook should return all the data the module wants to provide, every time.

I think this needs a big structural rethink. My suggestion would be to rewrite it as a plugin for Display Suite -- http://drupal.org/project/ds. That would take care of the exportability, and most of the UI, and would work across all entity types.

Automated Review

PAReview came up clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes.

No duplication

Maybe. The goal of the project seems to fall within the scope of Display Suite. The project need at least to make clear why this project is not a sub-module for Display Suite.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

No. The format of README.txt does not follow the guidelines for in-project documentation. In particular, see the README.txt Template.

Code long/complex enough for review

Yes. About 544 lines. Follows the guidelines for project length and complexity.

Secure code

Yes. The XSS vulnerability appears to be fixed. No more security issues identified.

Coding style & Drupal API usage

I found the following:

• (*) Several of your calls to variable_get does not provide a default value, which will produce an error when the variable requested does not exist. For instance, line 96 of field_concat_display.module is $concat_list = variable_get('field_concat_display_var_names');. If the variable 'field_concat_display_var_names' does not exist, this is NULL (which isn't handled). This scould have been: $concat_list = variable_get('field_concat_display_var_names', array());. Other places where this bug exists are: lines 15, 30 of field_concat_display.module, and lines 21, 293, 326 of field_concat_display.admin.inc.
• (*) On line 128 of field_concat_display.module, $extra is returned. This variable is only brought into scope if the array $concat_list is not empty. The way this is currently structured, you may return a variable that is not in scope.
• (*) The docblocks describing the functions does not have the tags @param and @return according to the API documentation and comment standards. This omission makes manual review difficult, as the reviewer must in some cases guess what the intent of function is.
• There is no hook_help(). It is good coding practice to have this hook for every enabled module.
• There are blank lines in field_concat_display.info. The standard format according to Writing module .info files does not have blank lines between the lines with a key/value pair.

The starred items (*) are fairly big issues and warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Changing status.

There are already fairly in-depth reviews by jeremyr, mpdonadio and joachim, so I think you've got all the attention the bonus tag affords you. I'm therefore removing the "PAReview: review bonus" tag. You may add it back after having done three more manual reviews.

The coding style issues from my last review seems to be sorted out. Below are some new findings:

Coding style & Drupal API usage

My notes:

• Having uppercase characters in a field name (E.g. MyField) does not validate (error message: "Field name must contain only alphanumeric characters and underscores." ) Uppercase is normally considered "alphanumeric".

  (Rather than using a home-cooked preg_match for validation, you should consider using the pre-cooked validation rules of the Form API Validation project.)

• (*) The README says: "The prefix/suffix textfields may be left blank". However, leaving them blank results in:
  

  Undefined index: prefix in field_concat_display_entity_view_entity()
  Undefined index: suffix in field_concat_display_entity_view_entity()
  

• (*) Clicking on the edit link for a single concat field gives a completely different UX from clicking on the same link for any other field. For "ordinary" field, this takes the user to a page where he can edit the field settings. For a concat field, it takes the user to a page showing all concat fields where it is possible to delete or update (but not add new fields to) an existing concat field, or create and save a new from scratch). This is very confusing. The UI should not introduce a whole new paradigm for the edit link for concat fields. Do not show an edit link for concat fields if it points to something that breaks the existing paradigm (The somewhat misnamed "Field Concat Display" tab gives the user access to the general management page for concat fields).
• The tab "Field Concat Display" is overloaded with three different functions: Delete, Update/Edit, and Create. This is not apparent from its name. Something more generic (e.g. "Mangage Field Concat Fields") should be less confusing.

The starred items (*) are fairly big issues and warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

manual review:

1. field_concat_display_entity_view(): no need to line break after $settings, code is allowed to exceed the 80 character limit of comments. Also elsewhere.
2. field_concat_display_delete_pseudo_field_callback(): this is vulnerable to CSRF attacks. Do not execute write operations such as deleting variables on GET requests without either using a confirmation form or a security token in the URL. See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained
3. field_concat_display_update_field(): check_plain() is wrong here: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://www.drupal.org/node/28984 . That means that you should do the escaping in field_concat_display_entity_view_entity() where you print the prefix and suffix into #markup.

Review of the 7.x-1.x branch (commit 0a71c9d):

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/field_concat_display.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   350 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
  --------------------------------------------------------------------------------
  
  Time: 107ms; Memory: 6.25Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. "$message = "Do you really want to delete '$node_type:$field'?";": all user facing text must run through t() for translation. Same for "drupal_set_message("Field: $field has been deleted.");", please check all your strings.
2. field_concat_display_confirm_delete_form(): this is still vulnerable to CSRF attacks. This function is a form constructor, so it should only create the form. The actual action should be done in a submit callback. By checking $form_state['input'] you are circumventing the Drupal form CSRF protection, so the automated test is right: do not use $form_state['input'] here.

manual review:

• field_concat_display_delete_pseudo_field_callback(): why do you need that function? You can just move the code to field_concat_display_confirm_delete_form_submit()? And you should not use drupal_goto() during processing of form submissions since that exits the execution and any possible other submit callbacks then don't get called. Use $form_state['redirect'] instead.

But otherwise looks good to me.

Assigning to dman as he might have time to take a final look at this.

I think this is good, and the cross-over with what can be done with display suite has been adequately addressed.
I'll give it a go..

This is a fresh-mind first-look at what this module does and how it works.

.. I think the project group (when enabling) should probably be "Fields" - not undefined. That's where I expect field rendering enhancements to be found. Not a biggie.

I made a new content type, starting with two fields (text + longtext)I wanted to glue together.

... I would have expected this to be a field rendering option using the field management UI ... but this is probably a preconception based on how I'm used to DisplaySuite presenting the option. Not convinced that adding a new tab *next to* instead of *inside* the field management options is the best way to make this useful. Have you seen the fun that can be had with field_formatter_settings ?

But using this UI, I guess it may still work. Feels cluttered as it's working around Drupal field display settings, not within them.
I selected two candidate fields, (a text and a longtext) and then the new settings page showed them to me with prefix and weighting options. OK. But did not save for me when I tried. I have to select the same two fields *again* ?
I was at /admin/structure/types/manage/test-concat/display
and the settings were troublesome/confusing.

Saving seemed to fail the first time, but seemed to take effect the second time.

However, hitting the "MANAGE 'FIELD CONCAT DISPLAY' FIELDS" tab again did *not* let me "manage" the existing field, it seemed to want me to add a new one, not edit the existing one.
While on the "manage fields" tab, I could only delete the one I made, not edit it. Are we expected to delete and make again every time we need to change?

I'm feeling the UI process is unfinished or untested.

Moving on to "manage display" I see the new combo field there. No display options, so I hope they are inherited from the settings for the individual fields themselves?
Of course, if I am "displaying" the combo field, I'll want to set the two originals to "hidden" So I do that (aI see this is suggested in the project page, so that makes sense).

Well, as a result, I do see the rendered output displaying the concatenated fields... but in one div, labeled class="form-item form-type-item" (which seems to be wrong classes) and with no whitespace between the two values, and no per-field wrapper divs or spans. This is a little bit difficult to see as useful for theming...

I guess it performs as described - to the barest minimum.

The long textarea I selected as the second field to concat seemed to lose its formatting. Which implies the display options I'd usually be able to manage for field rendering seem to have been ignored. HTML markup in the longtext was stripped?

I now have concerns about how well this will play with the other Drupal field formatter settings conventions - if it's going to strip tags and not use the other settings normally available with field display settings.

So, if there are multiple fields (a common situation)?
... Hm, well it does show them, but glued together with no space or wrappers.

I can't add, change or show a label on this new combo field, even if I want to, and that's a normal convention in Drupal field display management. It's OK if you expect it to be hidden by default, but it's non-drupally to remove the option altogether.

I tried adding a second combo field (longtext + image) to the same content type. That seemed to be possible (though the "MANAGE 'FIELD CONCAT DISPLAY' FIELDS" tab page still doesn't seem to make sense.
It did not show up in "manage fields" at first, but after some refreshes and re-saves I could find it. Have you tested adding two concats to one entity?

Concating an image field with a text field did not work - only rendered the text field. Does this not handle non-text fields at all, or are tags just getting stripped?

- So far I feel it's a bit of a basic effort so far. The code itself is fine I guess, and it could be used, but I can't see why or how this module could be useful yet. There are a number of small practical usability issues to sort out before it would be a release candidate for a general-use module.
* Firstly, even when concatenated, the two fields probably should still have some span tags around themselves. For theming and spacing.
* The rendering options for each field (by far one of the more powerful parts of D7 display management) should be respected or supported by this tool, not removed totally.
* are non-text fields unsupported by design?

... On review of this issue history, I see that the whitespace and strip_tags() choice has been mentioned in #15, #16. But the current status quo is still not useful.
and that the strange edit method through tabs and the inability to *edit* has been brought up by gisle in #36. In #37 you *removed* the edit link, which I don't think addressed the problem, and confused me. It's not a fix, it's a dodge.

Can't quite give this thumbs-up yet myself, it just did not seem usable. But I think some more user-testing would be able to refine it pretty easily.

So what happened here is that when I checked some boxes, and pressed save, the result was the same form, with the boxes I just checked empty.
In hindsight, I see that it did add the field - due to the presence of further settings in the table above, but the experience was unusual and didn't feel like what I expected had happened.

When I use the Drupal core field UI, I am totally used to creating a new field being a 2 step process.
* I'm creating a new field
* I get a form where I set base field settings
* on submission, I get a second form where I set field instance settings.
Both of those forms often have lots of settings I don't need to stop and think about, and can leave as default.

In the field_concat_display form pictured above, the new table of settings looked just like that:
* I'm creating a new field
* I get a form where I fill in the name and a setting
* on submission, I get a second form with settings I can probably leave as default
The second page there looked like a second form.

So now I stop to look at that form UI even more...

It confused me because it's going against existing Drupal conventions for how UI is expected to be presented. After studying it, I can see what you've done, but it doesn't match the expectations set by the core field UI...

To recover from this, a quick (though not complete) fixup may be to:
* show a message that the new field *has* been created
* show the field settings I just entered
* pull those buttons out of the table, where they do not belong
* maybe clean up that table some more

Maybe (and not neccessarily) something more like this:

... this is only HALF as good as it could be, but maybe you'll see what I'm meaning.

I think a lot of this UI, and the reinvention that you've done to manage it would have been avoided if this feature had been provided in a quite different way : by actually providing a new field type (such as in the add new field select) that did no data storage, and then just inserting its settings into the field UI settings subform.
Much less code and much easier to work with.

Yeah. I'm certainly guilty of just adding buttons and things because *I* know how to use them also. And the Drupal module collection has many many modules that get as far as "works for me" but don't get a lot of helpful UI hints or polish after that.

That's why in module reviews here - if the code is already fine I tend to do an open-mind narration of what was going through my head when I tried to use it.
What did I expect from the project description, what I thought when I started using it, what went wrong or seemed unintuitive, maybe some hints on how it could be easier to use for first-timers. To break us out of that "I know how it's expected to work" shell.

But don't take this UI stuff as a blocker, we all have different blind spots, and it may just be personal taste here coming through in my reactions. Remaking it to be a pseudo-field that gets treated like a field in the field UI as I expected ... would be some re-work.

But I do agree that the strip_tags bit is a strong count against it being useful for themers... I do think that needs fixing.

In the meantime however

The Code is fine, and the Drupal API usage is OK, and I don't feel the strong need to push this back into "needs work" again.
It does need work WRT to refining the UI and process, and I'd like to see that improved if you can, but the project application review process has been passed.

==========================

I updated your account so you can 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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.

Hi,
Can you please add a list of differences with computed field module https://www.drupal.org/project/computed_field so that people know exactely which one to choose depending on the needs.
Thanks
And congrats for this module contribution !