Legacy feature support in new stack user siteaccess usage #2

brookinsconsulting · 2015-04-04T23:35:03Z

Hello Netgen!

We again thank you for your fantastic solution! We took a moment to continue the work we started in #1

This pull request depends on #1 being merged first. Please merge the first pull request and then consider merging this pull request.

During the course of a normal share forum thread, http://share.ez.no/forums/ez-publish-5-platform/select-class-attribute-option-values-ez-5.4

We found several inconsistencies between the default legacy template feature support and the current new stack feature support. Basically the current new stack template can only display selected option identifiers and not the selected option names. I think we can agree that displaying option names is much more user friendly than only identifiers.

In general template logic (and underlying features) were quite different and also did not support the use of custom delimiter display.

The reason for the original limitations in the provided bundle was because the provided FieldType FieldValue Converter was so basic it did not provide support for the use of FieldSettings to the template layer with the stored ContentType Field settings.

In general we borrowed code and syntax for these additions for the default selection FieldType as a guide and customized provided FieldType FieldValue Converter using the legacy datatype's features (but not syntax) also as a guide (as required; not needed).

We have extended the solution in general to provide the same default user siteaccess features as legacy. We think this represents a substantial improvement to the new stack bundle that all user can benefit from, right out of the box! And while we have not ported all features of the legacy datatype specifically since eZ Platform UI is still alpha and feature incomplete (in terms of Content Type definition administration)... we think that our improvements provide a solid basis for current users trying to get the same features provided by the legacy datatype's default use provided feature set in a user siteaccess context and future developers extending the solution further (when eZ Platform UI does provide for Content Type definition administration).

This pull request solves issues surrounding different default use feature support.

We hope that you agree and will merge our improvements!

Note: This is our very first attempt at custom FieldType and Twig template bundle improvements based on legacy datatype and eztpl features. Please be gentle if we have made any errors or omissions :)

Please let us know how this finds you.

Take it eZ!

Cheers,
Brookins Consulting

brookinsconsulting · 2015-04-06T12:43:59Z

Ping @emodric

emodric · 2015-04-06T12:50:56Z

Thanks for this one!

However, since I'm traveling at the moment, please allow me a day or two to review this before merging. #1 was just merged, since it was a simple one!

brookinsconsulting · 2015-04-06T12:58:29Z

Hello @emodric

Thank you for your quick reply! Please take your time in reviewing and testing our improvements!

Safe travels :)

Cheers,
Brookins Consulting

emodric · 2015-04-20T08:12:48Z

Core/Persistence/Legacy/Content/FieldValue/Converter/EnhancedSelectionConverter.php

+        {
+            foreach ( $simpleXml->options->option as $option )
+            {
+                $options[(int)$option["id"]] = array( "id" => (string)$option["id"],


Wouldn't it make more sense for id and priority below to be integers?

Also, is there any specific reason why you set the array keys to id? Why not just use $options[] = array( ... )? I'm worried that someone somewhere will try to use this array as zero-based, while in reality it is be one-based.

Hello @emodric

Thank you for your detailed review. We are reviewing your comments in detail now and will address each of them with further responses and additional changes.

In general and in our defense we would like to say we were attempting to implement the core implementation of the existing legacy datatype as much as possible (where it makes sense).

Though upon review we have made some omissions and mistakes. Your comments have made this clear especially our error with $options[(int)$option["id"]] usage. We will address these with changes very shortly.

Cheers,
Brookins Consulting

emodric · 2015-04-20T08:18:31Z

Core/Persistence/Legacy/Content/FieldValue/Converter/EnhancedSelectionConverter.php

+        $fieldDef->fieldTypeConstraints->fieldSettings = new FieldSettings(
+            array(
+                "isMultiple" => $isMultiple,
+                "delimiter" => $delimiter,


You're not parsing the delimiter from the XML, like you're doing for multiselect and query.

Hello @emodric

We have addressed this issue with a new commit. Thank you for pointing out this bug. For some reason GitHub is not changing this code comment block as outdated so we made this comment.

emodric · 2015-04-20T08:21:21Z

Resources/doc/INSTALL.md

@@ -22,7 +22,7 @@ Add the following to your composer.json and run `php composer.phar update netgen

 ### Activate the bundle

-Activate the bundle in `ezpublish\EzPublishKernel.php` file.
+Activate the bundle in `ezpublish/EzPublishKernel.php` file.


You will need to rebase the pull request against the master to remove this diff, since it's already merged from your previous commit.

emodric · 2015-04-20T08:30:58Z

Resources/views/sckenhancedselection_content_field.html.twig

-    {% for identifier in field.value.identifiers %}
-        {{ identifier }}{% if not loop.last %}, {% endif %}
-    {% endfor %}
+    {% if available_options|length > 0 %}


Shouldn't this check if length of field.value.identifiers is greater than 0?

EDIT: I figured it out, it's because how options are stored in field settings. However, I would still check that both, available_options and field.value.identifiers are not empty.

Hello @emodric

In our initial testing we did not find this check as required. Today we realize this was because of our undocumented template code calling the field rendering.

{% if not ez_is_field_empty(content, "selection") %} {{ez_render_field(content, "selection")}} {% else %} Empty selection {% endif %}

See with the above code if field.value.identifiers is empty we would get the output 'Empty selection' instead as the conditional block (above) would never actually run the 'ez_render_field'.

We are modifying our calling template to remove the use of the conditional block (above) and directly just calling 'ez_render_field' during our testing and refactoring to address the issues you point our here.

Cheers,
Brookins Consulting

Hi @brookinsconsulting

I'm not really sure what was your point here?

All I was saying was that you should check if both fieldSettings.options and field.value.identifiers are empty before iterating over them, to make sure there are no template errors.

Also, I would not print No selection available if the field is empty, the choice of what should be printed out should be left to users, right?

My proposed version of template is this (I also incorporated using the delimiter throughout the template):

{% block sckenhancedselection_field %} {% spaceless %} {% set available_options = fieldSettings.options %} {% set identifiers = field.value.identifiers %} {% if fieldSettings.delimiter is not empty %} {% set delimiter = fieldSettings.delimiter %} {% else %} {% set delimiter = ',' %} {% endif %} {% if available_options is not empty and identifiers is not empty %} {% for option in available_options %} {% if option.identifier in identifiers %} {{ option.name }}{% if not loop.last %}{{ delimiter }} {% endif %} {% endif %} {% endfor %} {% elseif identifiers is not empty %} {% for identifier in identifiers %} {{ identifier }}{% if not loop.last %}{{ delimiter }} {% endif %} {% endfor %} {% endif %} {% endspaceless %} {% endblock %}

emodric · 2015-04-20T08:31:43Z

Resources/views/sckenhancedselection_content_field.html.twig

+                {{ option.name }}{% if not loop.last %}{% if fieldSettings.delimiter %}{{ fieldSettings.delimiter }}} {% else %}, {% endif %}{% endif %}
+            {% endif %}
+        {% endfor %}
+    {% else %}


Together with above comment, this else does not make sense any more, right?

emodric · 2015-04-20T08:36:05Z

Resources/views/sckenhancedselection_content_field.html.twig

+    {% if available_options|length > 0 %}
+        {% for option in available_options %}
+            {% if option.identifier in field.value.identifiers %}
+                {{ option.name }}{% if not loop.last %}{% if fieldSettings.delimiter %}{{ fieldSettings.delimiter }}} {% else %}, {% endif %}{% endif %}


You have an extra closing curly brace in {{ fieldSettings.delimiter }}}

Also, you should check if delimiter is not empty with {% if fieldSettings.delimiter is not empty %}

emodric · 2015-04-20T11:54:41Z

Hi @brookinsconsulting

Once again, thanks for this pull request :) Sorry for being late with the review, but you know how it is, client projects always come first :)

I've made some remarks on your code so check them out in your own time. If you don't have time to correct the issues, let me know, I'll merge the pull request and correct them.

Cheers!

brookinsconsulting · 2015-04-22T12:19:24Z

@emodric

Thank you for your replies. We will address them in detail shortly.

Cheers,
Brookins Consulting

brookinsconsulting · 2015-05-03T05:03:27Z

Hello @emodric

We have addressed your comments and concerns with some further improvements.

Please review our improvements and let us know what you think.

Note: We are considering further refactoring (in a separate PR #3 to provide a default template solution to sort the displayed selection options by priority (as stored / used in the FieldType options). We welcome your thoughts on this since we would require a twig filter to sort the fieldSettings options by 'priority'.
EDIT: The twig filter solution we were thinking of using turned out to be incomplete for the features required and was also license incompatible, so we created our own instead.

Cheers,
Brookins Consulting

brookinsconsulting · 2015-05-04T07:46:34Z

Hello @emodric

Apologies for the previous confusion. I have made the changes you suggest as they are what makes more sense and more readable.

I made one other change for simplicity of the potential user who creates a template override which does not want or need a space after the delimiter. I moved the space following the delimiter which was in the template logic into the default delimiter set statement instead.

I also made one other addition to the documentation to clearly explain the last undocumented step to getting their selection content displayed within their own custom template.

I have also merged this change into our other PR #3.

If you like I can flatten all of this PR's changes into a single commit.

Cheers,
Brookins Consulting

…upport for custom FieldSettings values to provide for new stack user siteaccess feature sets to match legacy user siteaccess provided template features and expanded documentation

emodric · 2015-05-04T09:30:34Z

Hi @brookinsconsulting

I've merged the pull request and did a couple of fixes in three commits, can you review them to see if there are any problems?

Also, can you now rebase your PR #3 again against master please?

brookinsconsulting · 2015-05-04T11:31:02Z

Hello @emodric

Thank you very much for your help with these improvements and for merging this morning!

We have rebased PR #3

Cheers,
Brookins Consulting

emodric reviewed Apr 20, 2015
View changes

brookinsconsulting force-pushed the brookinsconsulting:legacy_feature_support_in_new_stack_user_siteaccess branch from 071517a to 74e886d May 3, 2015

brookinsconsulting mentioned this pull request May 3, 2015

Add default support for sorting options by priority #3

Closed

Updated: Extended FieldType FieldValue Converter methods to provide s…

958ef8a

…upport for custom FieldSettings values to provide for new stack user siteaccess feature sets to match legacy user siteaccess provided template features and expanded documentation

brookinsconsulting force-pushed the brookinsconsulting:legacy_feature_support_in_new_stack_user_siteaccess branch from db8c17f to 958ef8a May 4, 2015

emodric merged commit 958ef8a into netgen:master May 4, 2015

brookinsconsulting deleted the brookinsconsulting:legacy_feature_support_in_new_stack_user_siteaccess branch May 4, 2015

netgen / NetgenEnhancedSelectionBundle

Legacy feature support in new stack user siteaccess usage #2

Legacy feature support in new stack user siteaccess usage #2

brookinsconsulting commented Apr 4, 2015

brookinsconsulting commented Apr 6, 2015

emodric commented Apr 6, 2015

brookinsconsulting commented Apr 6, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emodric commented Apr 20, 2015

brookinsconsulting commented Apr 22, 2015

brookinsconsulting commented May 3, 2015

brookinsconsulting commented May 4, 2015

emodric commented May 4, 2015

brookinsconsulting commented May 4, 2015

netgen / NetgenEnhancedSelectionBundle

Legacy feature support in new stack user siteaccess usage #2

Legacy feature support in new stack user siteaccess usage #2

Conversation

brookinsconsulting commented Apr 4, 2015

brookinsconsulting commented Apr 6, 2015

emodric commented Apr 6, 2015

brookinsconsulting commented Apr 6, 2015

This comment has been minimized.

emodric Apr 20, 2015 Member

This comment has been minimized.

emodric Apr 20, 2015 Member

This comment has been minimized.

brookinsconsulting May 3, 2015 Author Contributor

This comment has been minimized.

emodric Apr 20, 2015 Member

This comment has been minimized.

brookinsconsulting May 3, 2015 Author Contributor

This comment has been minimized.

emodric Apr 20, 2015 Member

This comment has been minimized.

emodric Apr 20, 2015 Member

This comment has been minimized.

brookinsconsulting May 3, 2015 Author Contributor

This comment has been minimized.

emodric May 4, 2015 Member

This comment has been minimized.

emodric Apr 20, 2015 Member

This comment has been minimized.

emodric Apr 20, 2015 Member

This comment has been minimized.

emodric Apr 20, 2015 Member

emodric commented Apr 20, 2015

brookinsconsulting commented Apr 22, 2015

brookinsconsulting commented May 3, 2015

brookinsconsulting commented May 4, 2015

emodric commented May 4, 2015

brookinsconsulting commented May 4, 2015

emodric Apr 20, 2015
Member

emodric Apr 20, 2015
Member

brookinsconsulting May 3, 2015
Author Contributor

emodric Apr 20, 2015
Member

brookinsconsulting May 3, 2015
Author Contributor

emodric Apr 20, 2015
Member

emodric Apr 20, 2015
Member

brookinsconsulting May 3, 2015
Author Contributor

emodric May 4, 2015
Member

emodric Apr 20, 2015
Member

emodric Apr 20, 2015
Member

emodric Apr 20, 2015
Member