[D7] Field Embed

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

Hi,

Please find some review comments which i felt like would be better to update.

1) In theme_field_embed can we try to return renderrable arrays and instead of use html markup we can try use theme_html_tag function.

2) In field_embed_form_alter i think it is better to add a check for form id

3) In field_embed_validate_fields line number 299 please add a isset check to avoid warning.

4) Line 308 add a check of if the input variable is an array

5) Same in template_process_field_embed line 211 add a isset condition before implode.

Please let me know if have any concerns.

I am getting following notices in your module when creating any node.

Notice: Undefined property: stdClass::$bundle_name in field_embed_validate_fields() (line 290 of C:\xampp\htdocs\dev725\sites\all\modules\field_embed\field_embed.module).
Notice: Undefined property: stdClass::$entity_type in field_embed_validate_fields() (line 290 of C:\xampp\htdocs\dev725\sites\all\modules\field_embed\field_embed.module).

Thanks

Wouldn't this be doable with https://www.drupal.org/project/token_filter?

Hello,

Your clone command should be: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/mredding/2294099.git
Otherwise we get prompted for a password. Also you should update your project name from 2294099 to field_embed.

field_embed.module
line 54: If you are just returning text, why take 6 additional parameters? You should remove the ones that are not being used.
line 65: Same issue as above.
line 76: Same issue as above.
line 90: You should check to make sure $matches is an array or at least $matches[0] exists.
line 139: extra newline character.
line 229: You should build a render array in this function. You want to avoid typing out HTML if possible. This is not manipulative without the use of a render array. No other modules or themes could alter this code.
line 251: extra newline character.

That is all I could find at first glance. Please let me know if you have any questions.

* For parameters you are using, you should add a @param to the documentation header.

1. Do you need this line because i think prepare is only needed if it is doing something:
-'prepare callback' => 'field_embed_filter_field_embed_prepare',

2. Be sure to avoid the word "and" and "or" in php.
And at all this line is completly unuseful because your $matches is always isset and you cannot call it if $matches is not an array as
you have declared the function as so.
- if (isset($matches) and is_array($matches)) {

3. You use often $matches[0] and $matches[1] which can result in notice

4. After enabling the module and added it for text formats then saved an empty body content it results with this:
Notice: Undefined index: #validate in field_embed_field_attach_form() (line 360 of E:\localhost\htdocs\drupal-test\sites\all\modules\contrib\field_embed\field_embed.module).
Warning: array_merge(): Argument #2 is not an array in field_embed_field_attach_form() (line 360 of E:\localhost\htdocs\drupal-test\sites\all\modules\contrib\field_embed\field_embed.module).

This is a startup review. I mean that when i stopped testing it after this errors so you can fix them and then i can continue

This is a pretty interesting idea. I can think of a few uses, such as adding an image field multiple times to a page and using multiple view modes to change the image style used.

Since the automated review has already been done, I thought that I would focus on some usability testing and manual code review.

Usability

My primary thought about the module is that discovering which fields are available requires some pretty technical knowledge of the node structure. What if the input filter help text was dynamic? As in something like this:

You may embed the following fields here:

- Tags: [[field:field_tags]]
- Image: [[field:field_image]]

You'd probably want to add some text about the optional view mode parameter, too, or even list them as well. And you probably wouldn't want to list the field into which you're about to embed another field, though it would be hard to detect. In fact, I wonder...

Ah. I see you check for that ('You cannot embed body inside itself'). Well done!

So I tested on an instance of simplytest.me using the core Article content type from the standard install profile.

It occurred to me that a common use case might be to hide the field (via Manage Display) and embed it in, say, the body field instead. When I tried this (I embedded field_tags into the body field), and then hid field_tags (via admin/structure/types/manage/article/display), I got a couple of errors:

• Notice: Undefined index: taxonomy_term in taxonomy_field_formatter_view() (line 1601 of /home/s4291a7bcbef258e/www/modules/taxonomy/taxonomy.module).
• EntityMalformedException: Missing bundle property on entity of type taxonomy_term. in entity_extract_ids() (line 7729 of /home/s4291a7bcbef258e/www/includes/common.inc).

When I tested with field_image, I noticed that the embed token [[field:...]] appears if the field has no value. Not sure what I'd expect, but not the token.

The field label also appeared when I tested with the image, even though it's set to be hidden in the view mode. I see that could be the job of the template, but even still, with the `$label_hidden` template variable, it should honor the hidden label setting of the view mode.

Manual code review

I see you have both theme_field_embed() and field-embed.tpl.php. Are both necessary?

The five level nested foreach loop in field_embed_field_form_validate() is insane! I think it's great that you hunt for self-referencing embeds, but you might consider searching for a way to validate only fields that have the input filter on them (e.g. textareas).

It also feels sketchy to add two properties (->entity_type and ->bundle_name) to all entities. Seems a bit intrusive and unwarranted for the objectives of this module. Again, it looks like this in service of the validation, so if there were, perhaps, a more targeted way to validate only fields that _could_ have embed codes in them, this would be unnecessary. At the very least, you might consider prefixing them with your module name (e.g. ->field_embed_entity_type) to avoid any potential conflicts.

That's what I got. Good luck! I could see using this on a project eventually.

Hi Michael,

Just went to take another look and noticed a new error on a clean install. I get it every time I create a new node of any type, even if it isn't using a text format with the field embed input filter on it.

Recoverable fatal error: Argument 1 passed to _field_embed_value_check_field() must be an array, string given, called in /home/s575153226a8af1e/www/sites/default/modules/2294099/field_embed.module on line 575 and defined in _field_embed_value_check_field() (line 554 of /home/s575153226a8af1e/www/sites/default/modules/2294099/field_embed.module).

Even before that error, I noticed that the input filter help text doesn't appear when creating new nodes. I wonder if the menu_get_object() call can't return anything unless one is editing an existing node.

Hi Michael,

So sorry about my delay in getting back to you. Just tested again on simpletest.me, again using the Article content type.

This module is pretty slick. There are no more errors, the help text is, well, really helpful, and embedding the image and term reference fields worked great for me.

One thing didn't work as expected: The tokens didn't render when I viewed the node on the home page as a teaser:

Hi Michael,

I hear you, and I know it must seem like I'm pushing back a bit much, but after a quick test, both Insert Block and Insert View process the embed token in the summary field. If they can do it, so can you.

But if you wanted to push back and add it as a known issue to your project, I could live with that.

Mainly it makes me wonder if your input filter is implemented properly, or if Insert Block and Insert View needed to somehow work around the issue.

Hey Michael,
Awesome module and excellent work following best practices. In order to get this module moving forward so you can have commit access on d.o, I have completed a PAReview as outlined at https://groups.drupal.org/node/427683 and attached a patch which addresses a few issues. Let me know what you think.
Thanks

Explanation of patch changes below:

 - Fixed a bug that appears when creating a new entity as shown below:
   Notice: Undefined variable: entity in field_embed_filter_field_embed_tips() (line 75 of field_embed.module).
 - Reformatted the array that outputs help text to use parameters instead of string concatenation in the t() function.
 - Rearranged the help text to display the full filter for formatter options in order to make it more clear and easier to cut and paste.
 - Changed the help text to only display formatter options for fields with multiple formatters since the default is redundant.
 - Changed references from node to entity in the help text.
 - Added sections to README.txt.
 - Added LICENSE.txt.

Hey Michael,
I did notice one other issue. On line 168 of field_embed.module, $original is undefined. I've tested this and it doesn't appear to affect the functionality but maybe just a simple return; line would work? According to php.net, the preg_replace_callback function should return the original string if no matches are found. See the Return Values section at http://php.net/manual/en/function.preg-replace-callback.php. I imagine if $original is not defined, it could potentially spew some messages in the log but I haven't noticed anything while testing. Do you know how to write simpletests? That may help get it to a fully baked state. But I think it might be good enough to submit as is. We can always add issues in the queue of other things we notice. I would say this is ready to go and RTBC.
Thanks

Fixing priority, "Major" is only for issues in Needs Review state that haven't gotten a review in at least 2 weeks. See https://www.drupal.org/node/539608#application-priorities

Blocking issues

1. Remove LICENSE.txt, the drupal.org packaging script will include all necessary licensing information.

I saw sure there was a module that did this or something similar, but I can't actually find any, so no duplication worries. I did not find any other issues. There's a few minor points listed at http://pareview.sh/pareview/httpgitdrupalorgsandboxmredding2294099git.

Hey Karl,

The PAReview was from back in August. Please make sure you review the latest code. Here are the results from a recent code sniff:

FILE: ./field_embed/README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
25 | WARNING | Line exceeds 80 characters; contains 89 characters
----------------------------------------------------------------------

Please do not let simple things like removing LICENSE.txt hold this back.

Thanks

Adding patch to remove LICENSE.txt

How is this module different from Token Insert or Token Insert Entity.
Please outline the differences (if there are any) in your project page and explain (if there are only a few) why its better to not provide the additional feature as a patch to said modules.

Hi Stefan,
Thank you for noticing and that is a very good question. The token modules you mention perform the same task but are different in their implementation. This module works as a text format filter rather than using token replacement. But it might be better to ask if this project is more similar to Entity Embed. See https://www.drupal.org/project/entity_embed.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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