Isn't it the schema that is wrong, or rather missing? Those look like they should be integers not strings. I see there is for example viewsreference.enabled_settings.limit which is set for integer. It will be that it also needs one for when it's used *view:?
> don't have the why on this part,
I can investigate further if absolutely necessary. But I think it is going to be because: (a) as mentioned, even if you index a field it is not used in the results from Search API (except in very specific Solr configurations); (b) the values that are used are from the loaded entity; (c) this does not yet include related entities, and in this case the geofield is on an entity referenced from the base entity.
> The issue occurs only when we use Search API Index based View.
Regarding the memory usage, as I see it here. I've looked at in the past, and did supply a patch or two that helped bring it down. But what I noted was:
First with Search API either DB or Solr unless you have done quite some work you will be doing an entity load for every row returned (so every point). You can do the work with Solr to make it use the field data and not do an entity load. This will reduce your memory usage significantly. There is a patch in Search API module I started to do the same with the db backend, but it's only a start, and requires much more work.
The second point where there is quite some memory usage is the same if you use views sql or search api, and probably doesn't come into play if you're talking just 1000 points or so. But I'll mention it here as I looked at this too at the same time. When constructing the points for display on the map there is a lot of configuration and the whole loop over all the points and creation of the output is quite intensive. This I don't see any way round, because doing anything else would loose the features of the module, being able to configure the output such. With flexibility comes some complexity.
Just a heads-up if anyone else notices this. I've not had time to investigate myself but we have a report that this patch broke a Search API based view https://github.com/localgovdrupal/localgov_directories/issues/378
Added Kernel tests to describe the functionality of the patch in https://git.drupalcode.org/issue/date_recur-3010184/-/compare/3.5.x...3.... The branch does not (yet) look at the views integration. I'll be honest I'm still a bit confused that it indexes the default version, and the latest version; and then only the default and latest. I guess this is where looking at the views integration comes in.
Additional explanatory note https://git.drupalcode.org/issue/date_recur-3010184/-/compare/3.5.x...3.... The existing tests passed with field values of cardinality 2, but storage of 1, because they were working on the inserted entity. But on loading (the default revision) from storage, of course, the second value was not loaded as it was not stored. Basically presently should you save an entity with a date recur field with more values than its cardinality it does calculate occurrences for the invalid deltas.
ekes → changed the visibility of the branch 3010184-create-occurrences-for to hidden.
BramDriesen → credited ekes → .
157 merged.
Many thanks.
https://github.com/localgovdrupal/localgov_guides/pull/155#issuecomment-...
> Is this a dupe of #145 ?
> If so if you could review that we can get it merged. If not maybe the two should be combined to add what ever got missed in the first?
I'm going to put this question here, as documentation, I couldn't find it anywhere else.
What is the minimum set of permissions for a solr role that will work out-of-the-box with the module? For example including the upload config. I see by default:
security-edit
security-read
config-edit
config-read
collection-admin-edit
core-admin-edit
core-admin-read
read
collection-admin-read
update
health
metrics-read
filestore-read
package-read
package-edit
schema-edit
schema-read
zk-read
all
I assume several of these that are needed could also be constrained to a collection.
@kerasai Did you continue to use this? Can you confirm that the 3.x compatible version works with 2.x as well? I'll change the dependency if so.
Aside: After all the work to make domain_group 3.x when group_sites came along we've decided it's worth switching to use it. Maintained by the Group module maintainer.
Fixed in 1.1.0-beta2 Thanks both.
Would still be nice to get some more tests over the new function that have been added.
Fixed in 1.1.0-beta2
ekes → created an issue. See original summary → .
(FWIW) Possibly not dissimilar use case, but we have the users with an admin role being able to log into additional admin domain which I ended up fixing in admin mode https://github.com/localgovdrupal/localgov_microsites_group/blob/4.x/src... The whole domain is then customised to make doing admin tasks easier.
DDEV is offered in addition to Lando. The project mentioned `composer create localgovdrupal/localgov-project` includes https://github.com/localgovdrupal/localgov_project/tree/3.x/.ddev
Assuming this is in the section that renames the local tasks which is the reason for the comment https://git.drupalcode.org/project/date_occur/-/blob/0.1.x/date_occur_ui... It's also partly the reason all that code is separate in the _ui module. It should fail gracefully however, and not explode. So the fixes there have been helpful. Not changing the local tasks names in this case isn't essential.
That said the parameter bag that it is being retrieved from is stored key => value, it seems convention that the parameter of the primary entity on the route will be keyed with the entity_type_id, and so far I've not seen an example for an entity route implementation that will fail on this. If there is one maybe can look to see if it can be made more robust.
Seems in moving the field storage id into the route parameters I'd not taking into account it was creating multiple routes (with different param values) but the same path. So opting (almost) for A putting back just the field machine name into the URL fixes this https://git.drupalcode.org/project/date_occur/-/commit/26b531bf11cb13bed...
Merged, along with a fix for 🐛 occurrence page crashes if there are more than one occurrence field on an entity type Active , should be safe to have multiple occurrence fields on different bundles of the same entity type.
Many thanks.
Ah! I thought you meant more than one field on the same entity type.
I'll look back at the original repo where I removed the field name from the path see what comment I made, and see if it makes sense to put it back for working out the field to load.
snowballstem.org is
The Snowball compiler translates a Snowball program into source code in another language - currently Ada, ISO C, C#, Go, Java, Javascript, Object Pascal, Python and Rust are supported.
So not PHP, and not the PHP stemmer this module uses https://github.com/wamania/php-stemmer
There are two options I can see.
- Convert the snowballstem algorithm into PHP and submit it to the PHP module.
- Or if people are going to use it: you can use the snowballstem library (which if I see the explanation it needs compiling and placing the .so file in you path) via PHP with https://github.com/amaccis/php-stemmer It would be possible to update this module to alternatively use that in addition
In the beginning, but not implemented, was the idea to also allow having augmenter entities. Rather than close, overrride and replacing in the index as is implemented, these would another entity type that just has the fields to add/replace data on the original. So the possibility is still there in the original architecture, but wasn't something that was needed for the project the module was originally written for.
Amusingly I think I did have the $field_id in the path at some point and refactored it out to simplify things. I think there might be other complexities involved with having multiple fields on the same entity. Is there a particular use use case for having more than on?
The
comment #7 →
is correct, and really if these are calculated it should result in them being calculated before the entity save when using the short $entity->field->value method for setting.
I've opened a new issue, rather than re-opening this rather old one, also as it's a bit more involved. It includes that it is recalculating it when it needn't be, and with a failing test in a branch here 🐛 Computed values Active
I'd say this is blocked on 🐛 Computed values Active
Pushed an update to the test in the branch which will fail because all the computed values are loaded from the db as NULL.
It uses a reflection class to get the field values from the entity as they came from the EntityLoad without doing any field loading.
// Before accessing the entity field and incidentally trigger a calculation.
$relection = new \ReflectionClass($entity);
$property = $relection->getProperty('values');
$property->setAccessible(TRUE);
// Retrieve the values as they came from storage.
$values = $property->getValue($entity);
The first test added passes as the $entity->geofield_field->value does set a wkt value to the value property and this is stored.
// Check stored set value.
$this->assertEquals($value, $values['geofield_field']['x-default'][0]['value']);
But the following tests all fail because while the property values have been loaded from the database, they are all NULL.
// Check stored computed values.
$geom = \Drupal::service('geofield.geophp')->load($value);
$centroid = $geom->getCentroid();
$bounding = $geom->getBBox();
$computed = [];
$computed['geo_type'] = $geom->geometryType();
$computed['lon'] = $centroid->getX();
$computed['lat'] = $centroid->getY();
$computed['left'] = $bounding['minx'];
$computed['top'] = $bounding['maxy'];
$computed['right'] = $bounding['maxx'];
$computed['bottom'] = $bounding['miny'];
$computed['geohash'] = $geom->out('geohash');
foreach ($computed as $index => $computed_value) {
$this->assertEquals($computed_value, $values['geofield_field']['x-default'][0][$index]);
}
Meh. Why did that pass D10.1? In the test setting the $field->value directly doesn't cause the calculation as setValue(). Maybe it should be really.
There are two parts to this I think. One loading the data that has been stored as single field data so it can be used rather than having to load entities to get it. The second to be able to store longer string data, as you can't retrieve the fulltext fields from the database being stored just as the token parts. So two patches to start to unpick this. Both at the least require tests. But the former, loading the field data is simpler. The later making a storage field probably needs more thinking. I'll push both as a branch on the issue too though, as it might be easier for others to also work from there? Finally I'd say documentation of how, and when, these should work will also be needed.
Loading field data
The first patch 3408375-03-load-index-field-data.patch adds an option, in much the same was as solr does (the configuration form stuff is c&p), to include the field data in the results. After which point if you do manage to configure the views fields correctly, and only use stored fields (yes not fulltext ones), the entities do not get loaded.
Storage field
The second patch 3408375-03-string-storage.patch adds a type only for storage. Nothing special is done, so it gets added as both a column on the search base table, and it's own table, I'm guessing this is underdesirable. It won't be searched, and if I read correctly, creating temporary tables with 'TEXT' types is (a bit, not as much as an entity load) slower. So it maybe only wants to be the additional table? I've not delved into what the queries look like when it's there. The other thing that would be handy is if solr and db implemented the same search api type (rather than having solr_ and db_ versions).
ekes → changed the visibility of the branch 3419108-pass-additional-parameters to hidden.
> What's the second branch for?
Automagically generated by Drupal.org I think, and then not hidden.
Replying from Slack:
I for sure can think of a few sites, date based things, where it would be helpful if the option was there to invalidate the cache if the list is updated and 'once a day' (or whatever fixed time period).
I made some code to help the backend of this https://www.drupal.org/project/referenced_entity_replace →
Merged this one in. Report certainly looking better. Let's see with the next commit (a code tidy).
FWIW in relation to the OP the decision was yes we should, and it's pretty much done bar the end-user testing, particularly upgrade path
https://github.com/localgovdrupal/localgov_microsites_group/pull/438
https://github.com/localgovdrupal/localgov_microsites_group/issues/429
🎉 nice one. Thanks.
Here goes:
Backward compatibility
The public method Geocoder::geocode does change its signature but only adding that it also accepts a GeocodeQuery in addition to a string https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34...
The reported change https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... in the ProviderBase::process is in fact one that was already the case before this patch, again it is additional, it was already also returning an Address Collection or Geometry. This is also the same for doGecode and doReverse. It's a Collection if the geocoder is from PHP-Geocoder https://github.com/geocoder-php/Geocoder/blob/9c2224fc81be843c5c8c3e8925... and Geometry if it's from geofield https://git.drupalcode.org/project/geocoder/-/blob/8.x-4.x/modules/geoco...
Added functionality
So in addition to the additional GeocodeQuery argument type for Geocoder::geocode method at the Provider level it adds explicit methods to use if you have constructed a GeocodeQuery yourself and want to run it directly with a specific geocoder provider. This makes sense because you often want to add particular specific arguments for specific geocoders in the GeocodeQuery. These are defined by the interface https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... which any provider supporting GeocodeQuery will implement. This is basically all of the PHP-Gecoder providers that return the AdressCollection, and not the GeometryProviderInterface based ones.
You see the interface and new methods in use in the Geocoder::geocode method https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... where it decides only to pass a GeocodeQuery into the provider if it implements the interface
Changes to how code is working internally
To enable reuse of the caching mechanism the methods to store have been spun out from ProviderBase::process. The internal methods ProviderBase::getCache https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... and ::setCache https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... are called in the ProviderBase::process https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... and it will operate in the same way as it did with the array containing a single string, or lon and lat https://git.drupalcode.org/project/geocoder/-/blob/8.x-4.x/src/ProviderB...
But now these methods are available for geocodeQuery https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... and reverseQuery https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... where they can pass in a GeocodeQuery. The reason to reuse the code in a method, in addition to DRY, was it would be easier to change the caching in the future as it's all in one method that everyone can use.
Testing
The test https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... call the Mock Provider https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... which if it the code described above is correct will get called and return a standard response. This is checking that the Provider's methods are actually getting called correctly if you are using the ::geocode, ::geocodeQuery etc. methods.
Usage
In the geo_entity module example also mentioned above https://git.drupalcode.org/project/geo_entity/-/blob/4bd966f02827113f4ed... the address and the country codes are made available as geodata, which are then used to customize the query depending on the provider.
For example for Nominatim it restricts the results to the country using its 'countrycodes' parameter https://git.drupalcode.org/project/geo_entity/-/blob/4bd966f02827113f4ed... No longer get results from the United States that include the string 'GB' when you know someone has created an address in Great Britain!
There are more optional parameters like the custodian_code for the https://github.com/localgovdrupal/localgov_os_places_geocoder_provider OS Places Geocoder that @progga has written. I listed other above in the motivation for this.
Passing these arguments in a GeocodeQuery to Geocoder::gecode where it runs multiple Providers is also safe, because the Providers ignore any arguments they don't know.
OK so `3406303-pass-additional-parameters` was the original, now merged again with 4.x at 4.20; and `3406303-additional-parameters-update` was my reapplying (I think correctly) to the updated 4.x with the two commits that were on 4.19, but that's not needed now. However for certainty you should see that there is no diff between the two branches (just different commits) :-)
ekes → changed the visibility of the branch 3406303-pass-additional-parameters to active.
ekes → changed the visibility of the branch 3406303-pass-additional-parameters to hidden.
I'll review this, but I think what's been committed is missing a lot of the patch that is required. Including that interface!
I stumbled on the fatal error when bumping 8.x-1.x up to HEAD (actually to get the fix for 🐛 Routing error with Drupal 10.2 (Route "entity.site_setting_entity_type.canonical" does not exist) Closed: duplicate / ✨ Support "Clone" functionality Fixed ). So yes I was fixing 8.x-1.x . l hadn't checked 2.x sorry.
Works, but it the hook_ should be ui_patterns_settings_ rather than ui_patterns_ as that's this module's name (rather than the one it depends on).
Meh. I can't reopen the issue, but it doesn't seem worth creating a new one for https://git.drupalcode.org/issue/site_settings-3398369/-/commit/696253fc... this. Hopefully you'll notice :)
I think that last patch got a bit over enthusiastic https://git.drupalcode.org/project/site_settings/-/commit/1bf2cdc17efe24... is still in use, but removed.
I'll add it to the (already merged) branch, and it could be merged again.
I've now got code using this, and it's doing just what's wanted there too: https://git.drupalcode.org/project/geo_entity/-/blob/4bd966f02827113f4ed...
Addressfield v2 works as well as v1, including with the different optional form wrappers. I'll add it to the composer.json as an option.
hestenet → credited ekes → .
Added to the branch.
> I was thinking it would be good if the `geocoder` service could accept GeocodeQuery.
I was trying to think of a way of making the change there backward compatible and that does seem to do it.
I'm thinking that ✨ Pass additional parameters to geocoders Needs review will probably fix this one for me. By being able to pass a Query it can have ::setLocal() added to it per query where the local used will be the language of the entity (rather than the interface language). So my German entity (or translation) can get 'Florenz' and my English one can get 'Florence' :)
Something like this?
https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34...
I'd kinda like a way of telling, as a consumer of the services, which type of plugin (instanceof Provider, or instanceof GeometryProviderInterface) it is; but I couldn't quite think of a way of separating them further without breaking backward compatibility. So this is trying to integrate with what's there in the same way it's working now.
I can see if I can create other issues with the caching. But in Geo entity → what we're doing is autocompleting the address, and geocoding a point at the same time, interactively in the UI. If you install it, enable the example module, and add languages to the site you can recreate this:
If you are in the site in English do a search for something in Florence it will cache this result - with the address. You then do the same search in Italian and it will return the English result - still with the address 'Florence' not (yes) 'Firenze'. And vice versa. Clear the cache. Do it in Italian first that Italian version of the address is cached.
Oop cached value in the browser. I'll make a follow-up. The specific issue Tanc reported is fixed.
Ah cool I'll have a look. I've got some time I can spend on this (or something else) so that's a bonus.
I think there are two answers to this.
One is that the photon provider should probably implement getHandlerWrapper() and check getCurrentLanguage() is in the list of known languages. If not is should fallback to a hard coded (or configurable) language.
The second is you don't always want to geocode for the getCurrentLanguage, even more so as it defaults to Interface language. You might know what the target language that you want results in... the specific use case I have in mind here is address completion for entities, and translation based on the entity language.
For this second more advanced case it would be helpful to be able to access the geocodeQuery method on the Provider. Then a Geocoder/Query (or specifically Gecoder/GeocodeQuery) can created by the caller withLocale() for that query. Doing this would also be an absolute bonus for geocoders that implement additional functionality (withBounds(), or non-standard parameters like the Nominatim provider's countrycode search restriction which requires withData() added).
Thinking aloud. It would feel like path of least resistance, as there are already providers for Gecoder PHP and GeoPHP, additional interface that Provider's (the Geocoder PHP ones) could implement. Just need thinking about how it works with the caching and throttling (though it might be nice to move that to - Drupal custom - Middleware but as suggested in Geocoder PHP's docs -- it's probably possible to keep the present set-up, though reading the code I realize the caching currently doesn't take into account the locale changing?).
Thoughts on both welcome. I kinda need the second so will be making some sort of patch I guess. If it's a good one that works for the module that'd be best!
Just looking at this as seem it's need for a project.
Applying to 11.x the failing hunk on 521 was a line of whitespace.
So uploading without the whitespace and setting back to previous status.
Added || ^3.0 in release 2.1.3
but then the /admin/people overview is not longer accessible.
Here with Open Social 12 and multiple new additional profiles /admin/people overview is now accessible with this patch. It includes duplicate lines for each profile. That's maybe outside of the requirement for Open Social to fix? It will require changing the view to list users, or just their primary profile.
Investigate the code and find other places with some specific profile processing and add profile bundle check to avoid any errors/confusions.
Did find another of these though. The profile preprocess was also throwing an exception for accessing a field on other profile bundles.
Updated patch against 12 with additional preprocess bundle check attached.
I believe this is caused by #3225869: Profile: Field field_profile_first_name is unknown → basically you can't add any other profile type because it's being assumed that it will have the first, last (and optionally nick) name fields. If the patch on the #3225869: Profile: Field field_profile_first_name is unknown → works for fixing this suggest closing it as a dupe of that one?
> maybe save your profile to see if that busts the cache (if it's a cache issue)
I found resaving my profile updated it.
Adding `accessCheck(TRUE)` is correctly re-implementing what is happening now. Looking at the code I think this should actually be `accessCheck(FALSE)`? Because I'm wondering if this does'nt want to avoid duplicates even if the user doesn't have access to it?
This patch, one way or the other, however is needed for D10.
Explicitly unsetting score makes sense.
Many thanks for catching that change in Search API and the patch. Bumping the required version should also work just fine.
https://git.drupalcode.org/issue/search_api-3353060/-/tree/3353060-db-lo... passes tests locally, and a very cursory test by usage. So for sure requires more testing and eyes on it. It does change the LocationAwareDatabaseInterface.
The logic has always been there since facet support was first introduced https://git.drupalcode.org/project/search_api_db/-/commit/275c74c6b6ebb4... although there it seems even clearer the purpose is to keep conditions and even sorts (which would in postgres later add another field iirc), so it's just removing unnecessary fields (like score... are there others?)
I can add:
I did try and write a (failing) test for this. There don't seem to be any tests for AND facets in search_api/tests at least, only OR? When I tried adding an AND facet in a Kernel test something like
// Search with a AND facet.
$location_options = [
[
'field' => 'location',
'lat' => '51.260197',
'lon' => '4.402771',
'radius' => '500',
],
];
$query = $this->buildSearch(place_id_sort: FALSE)->sort('location__distance');
$query->setOption('search_api_location', $location_options);
$conditions = $query->createAndAddConditionGroup('AND', ['facet:' . 'category']);
$conditions->addCondition('category', 'item_category');
$facets['category'] = [
'field' => 'category',
'limit' => 0,
'min_count' => 1,
'missing' => TRUE,
'operator' => 'and',
];
$query->setOption('search_api_facets', $facets);
$results = $query->execute();
I ended up with a PHPUnit\Framework\Exception: PHP Fatal error: Uncaught AssertionError: The container was serialized. in /var/www/html/web/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php:88
If you can make this into an issue fork I think it can be automatically tested. The linked coding standards, of which I think you touch on a few but not all, are from the automated tests.