[D7] Display Suite Inception

First of all, we need to fix the automatic basic review issues:

Please, take a look at: http://pareview.sh/pareview/httpgitdrupalorgsandboxstooit2196695git

EDIT: removed pareview.sh dump.

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

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.

Automated Review

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

• 
  FILE: /var/www/drupal-7-pareview/pareview_temp/ds_inception.class.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   1295 | ERROR | Unknown type hint "stdClass" found for $field_group
  --------------------------------------------------------------------------------
  
  
  

Manual Review

Add Implements hook_help() in your module file

@mudi: The wrong type hint on its own is surely not an application blocker, anything else that you found or should this be RTBC instead?

This module reminds me of Panopoly distribution but I think it's better given it is a simple module which adds features to Display Suite.

I have played with it and works like a charm. From my point of view, this module needs to improve its documentation to help users get used to it. I think that in README file there should be a link to the screencast because it is the fastest way someone can get started. A part form that, I think the hook_help should be implemented to add a help page inside drupal UI.

Manual Review

README.txt/README.md
[No: Does not follow] the guidelines for in-project documentation and/or the README Template:

• Provide a useful README-file
• Provide help text in the Drupal UI

I think this is not an application blocker, but it is a recommendation given the complexity of the module.

Keep up the good work!

Thank you for this module. I see myself using this!

Manual check:
Why are you prefixing functions like: _ds_inception_render_node()

I have tried installing the module twice, but every time I am getting this PDOException:

Column not found: 1054 Unknown column 'o.inception_type' in 'field list': SELECT o.inception_id AS inception_id, o.bundle AS bundle, o.inception_type AS inception_type FROM {ds_inception_overrides} o WHERE (o.bundle = :db_condition_placeholder_0) LIMIT 1 OFFSET 0; Array ( [:db_condition_placeholder_0] => page ) in _ds_inception_get_overrides() (line 577 of /var/www/nick-dev/sites/all/modules/ds_inception/includes/ds_inception.helpers.inc).

Describing the current ds_inception_overrides table, gives me an inception_entity_type, but not the inception_type

mysql> describe ds_inception_overrides;
+-----------------------+------------------+------+-----+---------+----------------+
| Field                 | Type             | Null | Key | Default | Extra          |
+-----------------------+------------------+------+-----+---------+----------------+
| id                    | int(10) unsigned | NO   | PRI | NULL    | auto_increment |
| type                  | varchar(255)     | NO   |     | NULL    |                |
| bundle                | varchar(255)     | NO   |     | NULL    |                |
| entity_id             | int(10) unsigned | YES  | MUL | NULL    |                |
| inception_id          | int(10) unsigned | NO   |     | NULL    |                |
| inception_entity_type | varchar(128)     | NO   |     | NULL    |                |
+-----------------------+------------------+------+-----+---------+----------------+
6 rows in set (0.04 sec)

The module however looks great, can't wait to try it out more.

Okay great, I am not receiving the previous error anymore, but still having some minor issues:

• Creating a new inception entity, gives following errors. It looks like the module is currently still having issues with multilingual websites, as my default profile is set up to be multilingual.

 179  30/Jan 09:11  info    sys  ds_inception module enabled.
                            tem
 180  30/Jan 09:13  notice  php  Notice: Undefined index: field_hide_title in locale_field_entity_form_submit() (line 438 of
                                 /var/www/inceptionds-dev/modules/locale/locale.module).
 181  30/Jan 09:13  notice  php  Notice: Undefined index: field_hide_breadcrumbs in locale_field_entity_form_submit() (line 438 of
                                 /var/www/inceptionds-dev/modules/locale/locale.module).
 182  30/Jan 09:13  notice  php  Notice: Undefined index: #language in locale_field_entity_form_submit() (line 438 of
                                 /var/www/inceptionds-dev/modules/locale/locale.module).
 183  30/Jan 09:13  notice  php  Notice: Undefined index: #language in locale_field_entity_form_submit() (line 438 of
                                 /var/www/inceptionds-dev/modules/locale/locale.module).
 184  30/Jan 09:13  notice  php  Notice: Undefined index: #language in locale_field_entity_form_submit() (line 438 of
                                 /var/www/inceptionds-dev/modules/locale/locale.module).
 185  30/Jan 09:13  notice  php  Notice: Undefined index: #language in locale_field_entity_form_submit() (line 438 of
                                 /var/www/inceptionds-dev/modules/locale/locale.module).
 186  30/Jan 09:13  notice  php  Notice: Undefined index: field_inception_merged_weights in locale_field_entity_form_submit() (line
                                 438 of /var/www/inceptionds-dev/modules/locale/locale.module).
 187  30/Jan 09:13  notice  php  Notice: Undefined index: field_inception_generic in locale_field_entity_form_submit() (line 438 of
                                 /var/www/inceptionds-dev/modules/locale/locale.module).
 188  30/Jan 09:13  notice  con  inception: added Startpage.

• In the Inception entity preview mode you can still drag around the fields, could be turned off.

• Changing the current layout to another layout gives me the following

Notice: Undefined index: field_hide_title in locale_field_entity_form_submit() (line 438 of /var/www/inceptionds-dev/modules/locale/locale.module).
Notice: Undefined index: field_hide_breadcrumbs in locale_field_entity_form_submit() (line 438 of /var/www/inceptionds-dev/modules/locale/locale.module).
Notice: Undefined index: field_inception_html in locale_field_entity_form_submit() (line 438 of /var/www/inceptionds-dev/modules/locale/locale.module).
Notice: Undefined index: field_inception_block in locale_field_entity_form_submit() (line 438 of /var/www/inceptionds-dev/modules/locale/locale.module).
Notice: Undefined index: field_inception_view in locale_field_entity_form_submit() (line 438 of /var/www/inceptionds-dev/modules/locale/locale.module).
Notice: Undefined index: field_inception_entity in locale_field_entity_form_submit() (line 438 of /var/www/inceptionds-dev/modules/locale/locale.module).
Notice: Undefined index: field_inception_merged_weights in locale_field_entity_form_submit() (line 438 of /var/www/inceptionds-dev/modules/locale/locale.module).
Notice: Undefined index: field_inception_generic in locale_field_entity_form_submit() (line 438 of /var/www/inceptionds-dev/modules/locale/locale.module).

• Changing layouts does not allow transferring fields from current regions to the new regions like in Display Suite.
  This would be another great addition to the module, the DS code should be reusable for this functionality.

• The code looks clean and safe + has sufficient comments in the include files.

Will definitely look forward using this module.

Hey Stuart,

Impressive module, DS is not my cup of tea, but I can review the code at least.

1. Do you really need to unconditionally include 7 files on every page request - http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/ds_inception.modu... - this will significantly increase memory, from memory node.pages.inc is massive. I am wondering if you can load these when needed? I would be interested in a PHP memory comparison before and after this module ;)
2. Super minor, but use SentEnCe case, try not to cap letters for fun http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/ds_inception.modu...
3. Similar to point 1, all menu callbacks should be in a .admin.inc file http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/ds_inception.modu.... The default type is MENU_NORMAL_ITEM, so there is a lot of redundant code here
4. Super nitpick, wrong comment style http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/ds_inception.inst...
5. It is good form IMO that all variables should be prefixed with the module name http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/ds_inception.inst... - you can see how this can go wrong
6. Instead of hook_form_alter() that runs on *every* form in the entire Drupal site http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/ds_inception.modu... try hook_form_FORMID_alter() that is more targeted.
7. This whole string concatenation with HTML is messy, use a theme function (and/or a template) http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/includes/ds_incep...
8. db_select() should be used where possible http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/includes/ds_incep... (on a related note you look like you are doing this query 3 times with a slightly different argument - function here?
9. Missing t() http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/includes/ds_incep...
10. user_access returns a bool, so this is weird http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/includes/ds_incep...
11. Loading 200 nodes here? http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/includes/ds_incep... This is a terrible idea. On most sites - this will lead to a PHP WSOD. Can you explain/fix this.

That is as far as I got in the review.

Asides from that, it would be good to list the modules that this module will not work with (and ones it does work with nice). I would imagine this will be the first issues in your issue queue otherwise. I see you are overriding the node page callback http://cgit.drupalcode.org/sandbox-stooit-2196695/tree/ds_inception.modu... this will surely make some other Drupal module incompatible.

Isn't this module's functionality similar to Page Manager en In-Place Editing? Perhaps edit this issue's summary and add an explanation as to why it's different/better, as suggested on https://www.drupal.org/node/1011698.

A detailed description of what your project does, including how it is different from other, similar projects, if applicable.

Automatic Review

Coder

ds_inception.js

Line 12: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
    function refreshUpdateArray() {
Line 192: Javascript strings should be passed through Drupal.t(). [i18n_3]
                                    alert("Error changing layout: " + html.message);
Line 216: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
    function handleDropzoneEvent(event, ui) {
Line 256: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
    function handleDragStart(event, ui) {
Line 266: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
    function handleDropEvent(event, ui) {

Manual Review

Individual user account

Follows the guidelines for individual user accounts.

No duplication

May cause module duplication and/or fragmentation. Might be similar to Page Manager en In-Place Editing. Please update the issue to explain the difference as

Master Branch

Follows the guidelines for master branch.

Licensing

Follows the licensing requirements.

3rd party assets/code

Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Does follow the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Follows the guidelines for project length and complexity.

Secure code

(*)DOES NOT meet the security requirements.

• Your input field to add classes on blocks had a XSS vulnerability. Simple enter '><script>alert('XSS attack!');</script><div class=' to demonstrate it.

I couldn't identify the specific code that should be changed, but I suppose you will know where to use filter_xss().

Coding style & Drupal API usage

1. (+) Complete the phpDoc with @return & @param statements (see https://www.drupal.org/node/1354#return). It's not entirely necessary for hooks or ctools plugin callback function etc., but is useful for your own functions.
2. I wouldn't start a function with an empty line. This is not common in core nor contrib modules. It's also not consistently done.
3. You don't need quotes for your description in your .info file.
4. I don't know the history and usage of this sandbox module, but I would think that if the module is not released yet it wouldn't need update hooks in the .install module.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

This review uses the Project Application Review Template.

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.