Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grimreaper created an issue. See original summary.

Grimreaper’s picture

Assigned: Grimreaper » Unassigned
Status: Active » Needs review
FileSize
3.12 KB

Here is the patch.

Thanks for the review.

Grimreaper’s picture

FileSize
3.35 KB

Also seing a missing "view_mode" in the schema of the leaflet_map plugin.

super_romeo’s picture

FileSize
2.9 KB
2.9 KB

Rerolled.

super_romeo’s picture

With my patch I have following errors:

Drupal\Core\Config\Schema\SchemaIncompleteException : Schema errors for core.entity_view_display.taxonomy_term.location_states.default with the following errors:
core.entity_view_display.taxonomy_term.location_states.default:content.field_geo_area.settings.multiple_map variable type is integer but applied schema class is Drupal\Core\TypedData\Plugin\DataType\BooleanData,

And same error for other fields:

  • hide_empty_map
  • disable_wheel
  • fullscreen_control
  • reset_map.control
  • map_position.force
  • leaflet_markercluster.control

Now we need hook_update_N to convert existing field values to bool type.

stephen-cox’s picture

Patch #5 didn't work for me. There where missing definitions for gesture_handling, icon.html and geocoder. Attaching patch for these.

itamair’s picture

On this I am worrying about the #5 comment on the need to hook_update_N to convert existing field values to bool type,
that eventually need to be packed with the patch itself.

Cannot juts we keep managing those settings as integer ones (instead to boolean)? and adjust the #6 patch accordingly ... ?)
I also need a RTBC to validate the last patch ...

Finn Lewis’s picture

Event with the patch from #6, leaflet_views schema still has problems.

Steps to reproduce:

1. Fresh install of Drupal 8.9.5, Leaflet 2.0.9
2. Add a geofield to a content type, create a leaflet_map view called 'test_leaflet'
3. Install config_inspector, go to /admin/reports/config-inspector and tick 'Only show errors'.
4. Click on 'List'
5. See the following on /admin/reports/config-inspector/views.view.test_leaflet/list

display.default.display_options.style.options.data_source 	Undefined 	undefined 	field_geofield 	missing schema
	display.default.display_options.style.options.entity_source 	Undefined 	undefined 	__base_table 	missing schema
	display.default.display_options.style.options.name_field 	Undefined 	undefined 		missing schema
	display.default.display_options.style.options.description_field 	Undefined 	undefined 		missing schema
	display.default.display_options.style.options.view_mode 	Undefined 	undefined 	full 	missing schema
	display.default.display_options.style.options.leaflet_map 	Undefined 	undefined 	OSM Mapnik 	missing schema
	display.default.display_options.style.options.height 	Undefined 	undefined 	400 	missing schema
	display.default.display_options.style.options.height_unit 	Undefined 	undefined 	px 	missing schema
	display.default.display_options.style.options.hide_empty_map 	Undefined 	undefined 	0 	missing schema
	display.default.display_options.style.options.disable_wheel 	Undefined 	undefined 	0 	missing schema
	display.default.display_options.style.options.fullscreen_control 	Undefined 	undefined 	1 	missing schema
	display.default.display_options.style.options.gesture_handling 	Undefined 	undefined 	0 	missing schema
	display.default.display_options.style.options.reset_map 	Undefined 	undefined 	<array> 	missing schema
	display.default.display_options.style.options.map_position 	Undefined 	undefined 	<array> 	missing schema
	display.default.display_options.style.options.icon 	Undefined 	undefined 	<array> 	missing schema
	display.default.display_options.style.options.path 	Undefined 	undefined 	{&quot;color&quot;:&quot;#3388ff&quot;,&quot;opacity&quot;:&quot;1.0&quot;,&quot;stroke&quot;:true,&quot;weight&quot;:3,&quot;fill&quot;:&quot;depends&quot;,&quot;fillColor&quot;:&quot;*&quot;,&quot;fillOpacity&quot;:&quot;0.2&quot;} 	missing schema

I've tried a bunch of things to suppress the errors as they also block our automated tests from passing.

It looks like some of these items are defined in https://git.drupalcode.org/project/leaflet/-/blob/2.0.x/modules/leaflet_..., so why the missing schema?

Any ideas most welcome!

Finn Lewis’s picture

Looking further at the config inspector, and comparing with another views style schema that does not produce errors (views_accordion) I see that the 'display.default.display_options.style.options' config looks to be defaulting to

label: Default style
type: views.style.*

See the following excerpt from config_inspector:

display.default.display_options.style 	Format 	mapping 	<array> 	
display.default.display_options.style.type 	Type 	string 	leaflet_map 	
display.default.display_options.style.options 	Default style 	views.style.* 	<array> 	
display.default.display_options.style.options.grouping 	Grouping field number %i 	sequence 	<empty> 	
display.default.display_options.style.options.data_source 	Undefined 	undefined 	field_geo 	missing schema
display.default.display_options.style.options.entity_source 	Undefined 	undefined 	__base_table

whereas the views_accordion config for display.default.display_options.style.options defines the expected style plugin:
display.default.display_options.style.options Views Accordion views.style.views_accordion <array>

So, is the leaflet_views schema somehow not defining the style configuration correctly?

The config schema appears to be falling back to 'views.style.*:' from

https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/modules/view...

Finn Lewis’s picture

I stared at this for so long, but it only started making sense when @progga pointed out the miss-spelling of 'leafet_map' rather than 'leaflet_map' on https://git.drupalcode.org/project/leaflet/-/blob/2.0.x/modules/leaflet_...

I've updated the config file in the patch attached also updating some of the default settings in /src/LeafletSettingsElementsTrait.php to ensure they are saved as FALSE rather than 0, which seems to be interpreted as an integer by the config_inspector. There may be more to this than I've seen, but the patch attached hopefully fixes our immediate failing tests.

Finn Lewis’s picture

itamair’s picture

@thanks Finn. I am not a Master on the schemas ... so I am relying on this contributions.
Are the 2 patches attached on the #10 identical?
Is any of them ok to be committed into dev???

Finn Lewis’s picture

After testing these patches, it is clear that the patch from #6 and the patch from #10 don't apply together, as the patch from #6 makes some changes to the leaflet_views config/schema file too.

Here's a patch that combines the changes in #6 and #10 and allows my local config tests to pass.

@itamair, let's get someone else to review this patch perhaps before committing to dev. I too am no expert on the config schemas, but I'm learning!

Finn Lewis’s picture

Version: 8.x-1.x-dev » 2.0.x-dev
FileSize
11.83 KB

Seeing an error when applying the patch from #13

leaflet-fix-config-schema-2883700-13.patch:227: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

Trying new patch to avoid that error.

Also changing version to 2.0.x-dev.

itamair’s picture

Ok thanks @finn. Let's keep this still in "Needs review" for a while.
I will try to review it if/when/as soon as I find some time ...

  • itamair committed ee345cc on 2.1.x authored by Finn Lewis
    Issue #2883700 by Finn Lewis, super_romeo, Grimreaper, stephen-cox: Fix...
itamair’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jbfelix’s picture

Hello,

For the views style plugins, this would be

mapping:
        force:
          type: boolean
          label: 'Force map center & zoom'
        center:
          type: mapping
          label: 'Centre'
          mapping:
            lat:
              type: float
              label: 'Lattitude'
            lon:
              type: float
              label: 'Longitude'

to keep decimals and a perfect centering.

Regards