Settings for width/height of iframe should allow non-numeric input

I believe that is incorrect. I think you can put something like "65%" in for the size for a dynamic map. Try it?

Looking at the display options for our formatter .....

the height and width of the map are labelled as being in pixels and we use a number render element ( restricted to positive integers )
to create a html element of type number.

so as it currently stands "65%" is not possible...

Hm, let's see.

So in both 7.x and 8.x, it looks like the .tpl.php or .html.twig template prints out whatever is in the width/height variable it gets as the width/height of the iframe, which determines the map size for the dynamic map.

So, what is getting passed in there? In 7.x, it just does a check_plain() on whatever was entered in the width/height settings field, and the settings field is just a textfield (not a number), so it could definitely be something like "75%".

But in 8.x, you are right: the settings field is restricted to be a number. That seems wrong to me, and I am not sure why it was ported that way. I think we should change it to be a textfield as it is in 7.x (which was done that way to support % widths for responsive design).

I am still forming an opinion - so I am just going to voice some thoughts

Regarding the use of the width and height in the html.twig template
There are two places were the variable is used.

(a) to set iframe attributes width and height.
(b) to create the src attribute for a static map definition (img element)

looking at (a) If I read the HTML spec correctly

HTML5 allows only "css pixels" .... are curious term, which we can read a px.
HTML4.0.1 permits px or percentages.

I imagine browsers will take a loose permissive approach here ...as the concern is always rendering what they are given, rather than being slaves to the spec.
As a module developer I would rather constrain our module to only producing output which will pass a html validator BUT I can see the point in supporting whatever the user want to do.

In short I want to allow px and %, but reject negative integers or strings like "300em", "300smarties" or "hello world"

For (b) the width and height must be in pixels and we must reject percent ....

(a) and (b) were recently separate variables ... and have only just been merged.
#2599664: HTML escaped text issue. perhaps that needs to be revisited.

An option we could explore here is setting the img width attribute as a percent but constraining the value used to form the src attribute.

So a partial todo list :-
We need a pull/down selector or a checkbox to select px or % ?
In the display options form we need to change to text to explicitly permit more than px.

I don't think this module has to be too smart about validation. This is a setting that is generally only done by admins. If they want to set the iframe width/height to something that browsers will not render properly for their site, that is their problem, not the module's problem, IMO.

So I think in 8 we should do what we did in Drupal 7, and allow admins configuring this field formatter to put in whatever they want, and just make sure it's not a XSS problem by check_plain or equivalent. If it doesn't work on their site, it is their own fault.

Here a patch which change form type to textfield for settings width and height.
this patch remove too most of deprecated functions SafeMarkup::checkPlain and add some suggestions based on node type and field_name used to fill the address.
Let's me know if you prefer a patch only on width and height settings, and that I create another issue for adding suggestions ?

Thanks for your review.

Thanks for the patch!

If you want to add a feature to this module (that it has more theme suggestions), yes please create a new issue and separate this out of the patch. Patches, in general (in all projects) should only contain changes relevant to the issue.

So, I reviewed your patch... a few comments:

1. +++ b/simple_gmap.module
   @@ -30,8 +30,20 @@
   +        'field_name' => NULL,
   

   So this change should go into your other issue.

2. +++ b/simple_gmap.module
   @@ -30,8 +30,20 @@
   +}
   +
   +/**
   + * Implements hook_theme_suggestions_HOOK_alter().
   + */
   +function simple_gmap_theme_suggestions_simple_gmap_output_alter(array &$suggestions, array $variables) {
   +  if ($node = \Drupal::routeMatch()->getParameter('node')) {
   +    $suggestions[] = 'simple_gmap_output__' . $node->getType();
   +    $suggestions[] = 'simple_gmap_output__' . $node->getType() . '__' . $variables['field_name'];
   +  }
   +  $suggestions[] = 'simple_gmap_output__' . $variables['field_name'];
   

   Also this in the other issue.

3. +++ b/src/Plugin/Field/FieldFormatter/SimpleGMapFormatter.php
   @@ -65,20 +65,18 @@
   -      '#description' => $this->t('Note that static maps only accept sizes in pixels'),
   -      '#min' => 1,
   -      '#step' => 1,
   +      '#description' => $this->t('You can set sizes in px or percent (ex: 600px or 100%)'),
   +      '#size' => 10,
   

   We should not lose the information here that static maps only work with pixel sizes. That is important.

4. +++ b/src/Plugin/Field/FieldFormatter/SimpleGMapFormatter.php
   @@ -65,20 +65,18 @@
   -      '#description' => $this->t('Note that static maps only accept sizes in pixels'),
   -      '#min' => 1,
   -      '#step' => 1,
   +      '#description' => $this->t('You can set sizes in px or percent (ex: 600px or 100%)'),
   

   Same here.

5. +++ b/src/Plugin/Field/FieldFormatter/SimpleGMapFormatter.php
   @@ -188,7 +186,7 @@
   -      $langcode = SafeMarkup::checkPlain($this->getSetting('langcode'));
   +      $langcode = ['#plain_text' => $this->getSetting('langcode')];
   

   Nice! I did not know about this feature, but this change record documents it:
   https://www.drupal.org/node/2559263

   So you can use #plain_text in a render array to indicate "Escape all tags".

   Technically this change is a bit out of scope for this issue too, but I think since it doesn't change behavior we can probably leave it here.

6. +++ b/src/Plugin/Field/FieldFormatter/SimpleGMapFormatter.php
   @@ -210,13 +208,14 @@
   +    $field_name = $items->getName();
   

   This goes into the other issue too.

thanks for you feedback.

Here a new patch only about map's width/height units. As static map only accept number (without "px"), I added 2 new parameters to set width and height of static maps. And so, we can set differents sizes if needed.

I will create another issue for adding theme suggestions. We can do this in a custom module, but then we need at least the field_name in the template's variables if we want to add a suggestion based on the field_name which handle the address.

It is extremely unlikely that someone would need to have both a static map and a dynamic map running off the same text field formatter. So, I would prefer to just leave it with one setting. This is something that a site builder sets up once and then can forget, so they can verify it is working, and fix it then if it isn't. I just don't see a strong need to have two separate settings, even though one is allowed to be just numbers and one you can put "px" or "%" after the number. So... I'll go ahead and set this to Needs Work.

A separate issue to add the field name to the template is fine. Actually we may already have an issue, that sounds familiar... yes:
#2262203: Add field context to theme variables array when viewing a field formatted with simple_gmap
Maybe you can take a look there and see if that patch meets your needs, or revise it or add to it, rather than starting a new issue?

Thanks!

OK, then, in your mind, we let only one settings width/height for both type maps, and we only document the two usecase in the settings description. I will review the patch.

A new attempt :-)

@flocondetoile

I could not get you last patch to apply, sorry

in response to #7

this patch removes too most of deprecated functions SafeMarkup::checkPlain

We have an issue for that, I have not gotten around to fixing it.. it has been on my mind.
I prefer your solution to mine. I am going to revisit the issue - create a patch with your fixes from here [ and give you patch credit. ]

Manipulating double escaping or XSS issues - Please forgive me I am a cautious bunny I want to see it debated and fixed in an issue all of it's own.

@martin107

Hum the patch apply well on my env. I will dig onto this. Have you others patchs applied on your module ?

For deprecated fucntions, OK, I will remove them from the patch. There is a last use of this function $url_value = urlencode(SafeMarkup::checkPlain($item->value)); I did'nt remove, because we can't use urlencode function on the render array. What we can do, in this case, is to use the same render array #plain_text and then apply the Twig filter url_encode on the variable $url_value inside the template.

go back to needs work then.

This patch applied well on dev. Removed the modifications about deprecated functions.

Sorry this is taking time to resolve.

There is a last use of this function $url_value = urlencode(SafeMarkup::checkPlain($item->value)); I did'nt remove, because we can't use urlencode function on the render array. What we can do, in this case, is to use the same render array #plain_text and then apply the Twig filter url_encode on the variable $url_value inside the template.

I propose we sort the urlencode issue out in the issue I have tagged.

+++ b/src/Plugin/Field/FieldFormatter/SimpleGMapFormatter.php
@@ -66,20 +66,18 @@
+      '#description' => $this->t('You can set sizes in px or percent (ex: 600px or 100%). Note that static maps only accept sizes in pixels, without the suffix px (ex: 600).'),

This is very nice. I think we should put this description into the 7.x version of this module as well.

To me, this patch seems to be clean and well done.

Yep this looks good,

Thank you everyone.

I am marking this fixed, and opening up a side issue for the backport.

Someone opened an issue to reverse this -- see #3035109: Field formatter: Percentage width and height settings are invalid
Just posting here so if anyone is still following this issue, they might be interested in the other one too.