Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EZP-21873 - Unable to use ez_render_field with ezgmaplocation_field #590

Merged
merged 1 commit into from Nov 4, 2013

Conversation

@pedroresende
Copy link
Contributor

pedroresende commented Oct 31, 2013

If you try to use

{{ ez_render_field( content, 'location' ) }}

on a gmaplocation_filed you'll get the following twig exception

 Key "width" for array with keys "" does not exist in EzPublishCoreBundle::content_fields.html.twig at line 306
500 Internal Server Error - Twig_Error_Runtime 

After investigating a bit more I found out that the problem is that the array parameters is empty

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Oct 31, 2013

@lolautruche
lolautruche reviewed Oct 31, 2013
View changes
eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig Outdated
{% set mapWidth = false %}
{% else %}
{% set mapWidth = parameters.mapWidth|default( defaultWidth ) %}
{% endif %}

This comment has been minimized.

Copy link
@lolautruche

lolautruche Oct 31, 2013

Contributor

Why adding an extra if here ? The original {% set mapWidth = parameters.width|default( defaultWidth ) %} was sufficient.

@lolautruche
lolautruche reviewed Oct 31, 2013
View changes
eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig Outdated
{% set mapHeight = false %}
{% else %}
{% set mapHeight = parameters.height|default( defaultHeight ) %}
{% endif %}

This comment has been minimized.

Copy link
@lolautruche

lolautruche Oct 31, 2013

Contributor

Same remark as above.

@pedroresende

This comment has been minimized.

Copy link
Contributor Author

pedroresende commented Oct 31, 2013

@lolautruche I've moved the if there due to the field description

{##
 # This field type block accepts the following parameters:
 # - string mapType the Google map type (ROADMAP, SATELLITE, HYBRID or TERRAIN), default is ROADMAP
 # - boolean showMap whether to show the Google map or not, default is true
 # - integer zoom the default zoom level, default is 13
 # - string|false width the width of the rendered map with its unit (ie "500px" or "50em"),
 #                      set to false to not set any width style inline, default is 500px
 # - string|boolean height the height of the rendered map with its unit (ie "200px" or "20em"),
 #                         set to false to not set any height style inline, default is 200px
 #}

As you can see the width and the height can be a boolean and a string in case you use a unit value

@@ -303,15 +303,24 @@

{% set zoom = parameters.zoom|default( defaultZoom ) %}
{% set mapType = parameters.mapType|default( defaultMapType ) %}
{% if parameters.width is sameas(false) %}

This comment has been minimized.

Copy link
@andrerom

andrerom Nov 1, 2013

Member

I think @lolautruche is talking in the direction of just changing this line to also check if defined before checking it is false.

@pedroresende

This comment has been minimized.

Copy link
Contributor Author

pedroresende commented Nov 2, 2013

@andrerom, @lolautruche you're both right, just fixed as commented and rebased. Ready to be merged

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Nov 3, 2013

General note here. Ideally, this patch should be combined with #585

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Nov 4, 2013

+1

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Nov 4, 2013

@lolautruche Isn't the "false" handling broken now in current state of patch? (334ad02)

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Nov 4, 2013

Mmm I think you're right.

Suggestion:

{% set mapWidth, mapHeight = defaultWidth, defaultHeight %}
{% if parameters.width is defined %}
    {% set mapWidth = parameters.width %}
{% endif %}

{% if parameters.height is defined %}
    {% set mapHeight = parameters.height %}
{% endif %}
@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Nov 4, 2013

@pedroresende You'll update and ammend?

@pedroresende

This comment has been minimized.

Copy link
Contributor Author

pedroresende commented Nov 4, 2013

@andrerom Sure, I'm on it

@pedroresende

This comment has been minimized.

Copy link
Contributor Author

pedroresende commented Nov 4, 2013

@andrerom done

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Nov 4, 2013

+1

lolautruche added a commit that referenced this pull request Nov 4, 2013
Fix EZP-21873 - Unable to use ez_render_field with ezgmaplocation_field
@lolautruche lolautruche merged commit 3b0fadc into ezsystems:master Nov 4, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Nov 4, 2013

Thanks @pedroresende & lolautruche !

@pedroresende pedroresende deleted the pedroresende:EZP-21873 branch Nov 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.