Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Melissamcewen created an issue. See original summary.

stefan.r’s picture

Entity reference support is currently very rudimentary, we welcome any patches that improve upon it!

dkhalabuda’s picture

Status: Active » Needs review
FileSize
37.55 KB

This issue has been fixed. I copied over the .module file for term reference (hs_taxonomy) to entity reference (hser) and updated the way vocabularies are retrieved and set to utilize entity reference.

manningpete’s picture

Status: Needs review » Needs work

That patch creates an error when trying to apply via Simplytest.me.

manningpete’s picture

I'm uploading Dmitry's reroll to see it applies via simplytest.me

manningpete’s picture

Status: Needs work » Needs review

It does apply, and does what it says it does (provides configuration to either require deepest level of taxonomy, or select any level of taxonomy). Changing to Needs Review.

josephdpurcell’s picture

EDIT: after talking with Dmitry I realize almost all this code is copy and pasted. Hence, I'm removing my comments about code quality since that is out of scope--if the code style fixes are fixed here they should also be fixed elsewhere.

josephdpurcell’s picture

Confirmed works:

  • The "level choice" appears to work correctly: forcing the user to choose a deepest level auto selects to the deepest level, thus forcing the user to also do so.
  • The "Resizable" flag works: when disabled the resize bar doesn't show.
  • Two Dropbox settings appears to work: I can add and remove up to the specific limit and change the title
  • Editability settings appear to work: you can allow creation of new terms, limit particular levels from creating, allow creation of new levels, and set the max number of levels allowed

Issues:

  1. While I suspect there isn't a loss of data, when switching between the two "Save lineage" options the select box becomes unclear what is selected. For example, when going from "Save term lineage" to "Save only the deepest term", the select box goes form showing all 3 levels to only 2 levels. And to get to the 3rd level again you have to select off what was previously chosen for the 2nd level and re-select it.
  2. The "Level Labels" does something odd: when the labels are enabled they become options in the list, thus introducing new values to the input which I don't believe is desired.
  3. Dropbox settings that don't appear to work:
    • it doesn't look like "Reset selection of hierarchical select" works: after disabling it continues to reset the selection after adding items
    • The "Sort dropbox items" does not seem to stop sorting even when disabled

It may be that these issues existed previously, in which case perhaps we create a separate ticket to address these issues. If they didn't pre-exist, we should fix them now.

So, next step: verify they didn't exist previously.

josephdpurcell’s picture

  1. This was a pre-existing issue.
  2. This was a pre-existing issue.
  3. Dropbox settings:
    • "Reset selection of hierarchical select" did not work correctly previously
    • On second review, it looks like sorting does work!

I think the state of functionality is acceptable.

stefan.r’s picture

So, RTBC?

josephdpurcell’s picture

Looking at the code, here is the phpcpd output with the patch applied:

Found 5 exact clones with 291 duplicated lines in 2 files:

  1.	modules/hser/hser.module:397-448
 	modules/hs_taxonomy.module:402-453
 
  2.	modules/hser/hser.module:616-646
 	modules/hs_taxonomy.module:617-647
 
  3.	modules/hser/hser.module:820-941
 	modules/hs_taxonomy.module:873-994
 
  4.	modules/hser/hser.module:926-966
 	modules/hs_taxonomy.module:1075-1115
 
  5.	modules/hser/hser.module:966-1015
 	modules/hs_taxonomy.module:1115-1164
 
4.39% duplicated lines out of 6630 total lines of code.

Here is some commentary on each:

  1. This duplication is for logic for "Collect every possible term attached to any of the fieldable entities." which is copied from hs_taxonomy_field_formatter_prepare_view. The refactoring I might suggest is:
    1. Extract lines 407-446 in the hs_taxonomy_field_formatter_prepare_view function in modules/hs_taxonomy.module into a separate function inside hierarchical_select.module.
    2. Then refactor hs_taxonomy_field_formatter_prepare_view, hser_field_formatter_prepare_view, and hs_taxonomy_field_formatter_prepare_view to use this new function.
  2. This duplication is for hook_hierarchical_select_root_level(). The refactoring I might suggest is:
    1. Combine _hser_hierarchical_select_get_tree and _hs_taxonomy_hierarchical_select_get_tree into a single function in hierarchical_select.module, then delete those functions.
    2. On this new function from 2.1, add an additional parameter called something like "module_prefix" that gets passed as a tag on the query as seen in hierarchical_select.module::1114 and modules/hser/hser.module::965.
    3. Refactor all calls to the two previous functions to this new function from 2.1, passing the new "module_prefix" parameter.
    4. Combine the contents of hser_hierarchical_select_root_level and hs_taxonomy_hierarchical_select_root_level into a single function in hierarchical_select.module, but keep those functions.
    5. Have this new function in 2.4 call the new function from 2.1.
    6. Update hser_hierarchical_select_root_level and hs_taxonomy_hierarchical_select_root_level to call the new function from 2.4.
  3. There would be several different refactorings involved here:
    1. hook_hierarchical_select_config_info:
      1. Combine hser_hierarchical_select_config_info and hs_taxonomy_hierarchical_select_config_info into a function in hierarchical_select.module, but keep the functions.
      2. In the new function, eliminate the use of a static and have it simply iterate over the bundles and comparing the field instances to the widget type passed in.
      3. Refactor the previous two hooks to call this new function and use a static for caching (keeping in mind that statics are evil).
    2. Combine _hser_token_termpath_for_vid and _hs_taxonomy_token_termpath_for_vid into a single function in hierarchical_select.module and add a paramter "module" that gets passed to the $config['module'] value; then update all calls to use the new functions
    3. I think theme_hser_formatter_lineage and theme_hs_taxonomy_formatter_lineage are fine to keep separate, i.e. no refactor
    4. Combine _hser_hierarchical_select_get_tree and _hs_taxonomy_hierarchical_select_get_tree as described in 2.1
    5. Combine hser_get_config_id and hs_taxonomy_get_config_id into a single function in hierarhcical_select.module, then delete these two functions
    6. Combine hser_term_count_nodes and hs_taxonomy_term_count_nodes into a single function in hierarhcical_select.module, adding a parameter called "module_prefix"; refactor this new function to call the new get_tree function from 2.1, and refactor the tag on the query to be something like addTag($module_prefix . '_term_count_nodes')
    7. Combine _hser_hierarchical_select_terms_to_options and _hs_taxonomy_hierarchical_select_terms_to_options into the same function in hierharchical_select.moodule, then delete this functions
    8. Combine _hser_hierarchical_select_get_depth and _hs_taxonomy_hierarchical_select_get_depth into the same function in hierarchical_select.module, then delete these functions, add a parameter "module prefix", refactor the "get_tree" call to be the new function from 2.1, passing the module prefix there
  4. Steps for this are combined with #3.
  5. Steps for this are combined with #3.

In summary, there are some easy wins like 3.7, but overall I don't know that this much refactoring is within scope of this ticket.

josephdpurcell’s picture

Status: Needs review » Reviewed & tested by the community

I'm going to mark this as RTBC with the expectation that we can address the pre-existing functionality issues in #8 and the code duplication in #11 in a separate ticket.

Melissamcewen’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
73.37 KB
547.77 KB

Unfortunately it breaks features- see attached screenshots

josephdpurcell’s picture

Both errors can be seen here: /admin/structure/features/create

There is a third error that can be seen at: /admin/structure/types/manage/article/fields/field_primary_taxonomy, which is for editing a field instance that is an entity reference targeting taxonomy terms.

And a fourth error that can be seen at: /admin/structure/types/manage/article/fields/field_test2/widget-type, which is for editing a field instance that is an entity reference targeting nodes.

The errors are caused by these lines:

  1. +++ b/modules/hser/hser.module
    @@ -130,3 +113,992 @@ function hser_node_validate($node, $form, &$form_state) {
    +            $machine_name = $field['settings']['allowed_values'][0]['vocabulary'];
    

    This is the culprit for: Notice: Undefined index: allowed_values in hser_hierarchical_select_config_info() (line 818 of /var/www/docroot/sites/all/modules/contrib/hierarchical_select/modules/hser/hser.module).

  2. +++ b/modules/hser/hser.module
    @@ -130,3 +113,992 @@ function hser_node_validate($node, $form, &$form_state) {
    +              'hierarchy'      => t('Vocabulary') . ': ' . l(t($vocabulary->name), "admin/structure/taxonomy/$vocabulary->machine_name") . '<br /><small>' . t('Field label') . ': ' . $instance['label'] . '<br />' . t('Field machine name') . ': ' . $field_name . '</small>',
    

    This is the culprit for: Notice: Trying to get property of non-object in hser_hierarchical_select_config_info() (line 824 of /var/www/docroot/sites/all/modules/contrib/hierarchical_select/modules/hser/hser.module).

  3. file: modules/hser/hser.module
      61             'status' => $instance['widget']['settings']['editable'],
    

    This line the culprit for: Notice: Undefined index: editable in hser_field_widget_form() (line 61 of /var/www/docroot/sites/all/modules/contrib/hierarchical_select/modules/hser/hser.module).

  4. file: modules/hser/hser.module
     167     $vocabularies = $field['settings']['handler_settings']['target_bundles'];
    

    This line the culprit for: Notice: Undefined index: target_bundles in hser_form_field_ui_widget_type_form_alter() (line 167 of /var/www/docroot/sites/all/modules/contrib/hierarchical_select/modules/hser/hser.module).

The common theme is that the widget "settings" property seems to not be getting populated with the correct indexes, e.g. "editable" and "allowed_values". Why that is will require further investigation.

Edit 6/7: Added a fourth error.

dkhalabuda’s picture

Status: Needs work » Needs review
FileSize
35.88 KB

This patch fixes the errors by modifying the way vocabularies are retrieved on the configuration page.

josephdpurcell’s picture

+++ b/modules/hser/hser.module
@@ -130,3 +113,994 @@ function hser_node_validate($node, $form, &$form_state) {
+            if ($instance['widget']['type'] == 'hser_hierarchy' && $field['settings']['target_type'] == 'taxonomy_term') {

Why are only entity references fields that reference taxonomy terms being exported? I would expect any entity reference using hierarchical select to get exported here.

dkhalabuda’s picture

Updated patch. This will now display all entity references on the HS configuration page.

josephdpurcell’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me and resolves the errors identified in 13 and 14. Setting RTBC.

  • stefan.r committed 095e349 on 7.x-3.x authored by dkhalabuda
    Issue #2721829 by dkhalabuda, Melissamcewen, josephdpurcell: Allow term...
stefan.r’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks!

Melissamcewen’s picture

Status: Fixed » Needs work

Unfortunately we discovered this patch has issues in our production environment. It prevented users from adding multiple terms even though the field allows it- when multiple terms already existed, it deleted them.

I wouldn't roll this in unless it has associated tests and they pass. Also I noticed some of the basic unit tests already in the module aren't passing even w/o this patch.

manningpete’s picture

Steps to reproduce

  1. Apply #17 to hierarchical_select-7.x-3.0-beta7
  2. Create following "Colors" vocabulary
  3. - Red
    -- Crimson
    -- Blood
    --- Dried
    --- Fresh
    - Green
    - Blue

  4. Enable Hierarchical Select, Hierarchical Entity Reference modules (along with their dependencies
  5. Add a Color field to article content type, entity reference type, hierarchical select widget
  6. Target Taxonomy term > Color > Save
  7. Note: " Notice: Undefined index: editable in hser_field_widget_form() (line 61 of /blabla/sites/default/modules/hierarchical_select/modules/hser/hser.module)."
  8. on /admin/structure/types/manage/article/fields/field_color, set number of values to "Unlimited"
  9. Create and article and try to assign mulitple colors and you can't.
manningpete’s picture

Upon further investigation, the existing multiple terms were not deleted from the database. Unrelated to this patch, many of the nodes themselves had been deleted. Once I updated to hierarchical_select-7.x-3.0-beta7 (without the patch), the existing multiple terms are showing again.

dkhalabuda’s picture

The settings have to be updated to "Enable the Dropbox" in the Widget Type settings: admin/structure/types/manage/$field_type$/fields/$field_name$/widget-type
Please confirm.

  • stefan.r committed 158e2c0 on 7.x-3.x authored by dkhalabuda
    REVERT Issue #2721829 by dkhalabuda, Melissamcewen, josephdpurcell:...
m.lebedev’s picture

m.lebedev’s picture