rupertj → created an issue.
o/
Hello.
I was there
o/
Done!
I had the same reaction to checkboxes when I found the flaw!
Filtering those values out in the form makes a lot of sense. I'll move it there.
I've pushed that change to the MR. It fixed the issue for me.
The array_filter takes out all the checkbox values that didn't get chosen. Without that, they're still there and show up as clashing bundles. (See the screenshots I just added.)
I can't recreate this on a plain install now, although I did on an existing site build earlier in the week. I'll close this and re-open a new issue if required.
So my comment in #7 managed to derail the original intent of this issue. I should have made a new issue for that really...
Anyway, I still get the original reported issue, and MR 3 still fixes it, so I'm going to re-open this issue to evaluate and merge MR 3.
Yes, but is that a problem? It's what currently happens in directories at the moment...
I've added before and after screenshots to the attached files.
The styling was actually nicer before I went back and removed some references to directories, but I think that's right, as the styling should be re-added specifically for finders.
aaron.hirtenstein → credited rupertj → .
It looks like this was fixed in #3502083 and already merged.
I’m still interested - I just didn’t realise this issue was waiting on me.
Assigning to me as I've got a fix for this locally to push.
I've pushed my code to the issue fork, but I'm starting to think it could be overkill, so I'm keen to hear what people think.
It's based on what localgov_directories does. It implements hook_pathauto_pattern_alter() to add the channel URL to an entry URL if the entry is in a channel. The thing is, with finders, the content types set up as entries have to be in a channel, so we don't need to do this exactly like directories.
I've also tried out just setting the pathauto patterns appropriately, and that works.
EG, we could set up localgov_events like this:
- Event channel: /events/[node:title]
- Event: [node:finders_channels:0:entity:url:relative]/[node:title]
Letting localgov_events set those patterns and freeing finders from the responsibility of doing anything with URL aliases could be an option.
Either option requires this patch to token to let it use bundle fields: https://www.drupal.org/files/issues/2024-06-27/token-consistent-entity-a... →
Yep. It was that.
Actually, this could be me missing things from the config in localgov_events.
finn lewis → credited rupertj → .
Facets doesn't have the same issue, oddly:
% ddev drush en facets
[success] Module facets has been installed. (Help - Permissions - Configure)
% ddev drush en finders_facets
In RouteProvider.php line 211:
Route "entity.finders_facets_type.collection" does not exist.
Failed to run drush en finders_facets: exit status 1
What you're actually meant to do with those (Help - Permissions - Configure) "links" that appear in the terminal, I don't know.
It's drush 13.3.3.0, and the command was "drush en finders_facets", although enabling localgov_events 4.x which has finders_facets as a dependency triggers the same error.
Here's the stack trace from when the exception's thrown:
RouteProvider.php:211, Drupal\Core\Routing\RouteProvider->getRouteByName()
RouteProviderLazyBuilder.php:83, Drupal\Core\Routing\RouteProviderLazyBuilder->getRouteByName()
UrlGenerator.php:443, Drupal\Core\Routing\UrlGenerator->getRoute()
UrlGenerator.php:276, Drupal\Core\Routing\UrlGenerator->generateFromRoute()
MetadataBubblingUrlGenerator.php:108, Drupal\Core\Render\MetadataBubblingUrlGenerator->generateFromRoute()
Url.php:765, Drupal\Core\Url->toString()
PmCommands.php:112, Drush\Commands\pm\PmCommands->Drush\Commands\pm\{closure:/var/www/html/vendor/drush/drush/src/Commands/pm/PmCommands.php:111-113}()
PmCommands.php:111, array_map()
PmCommands.php:111, Drush\Commands\pm\PmCommands->install()
CommandProcessor.php:276, call_user_func_array:{/var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php:276}()
CommandProcessor.php:276, Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback()
CommandProcessor.php:212, Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter()
CommandProcessor.php:175, Consolidation\AnnotatedCommand\CommandProcessor->process()
AnnotatedCommand.php:387, Consolidation\AnnotatedCommand\AnnotatedCommand->execute()
Command.php:326, Symfony\Component\Console\Command\Command->run()
Application.php:1096, Symfony\Component\Console\Application->doRunCommand()
Application.php:324, Symfony\Component\Console\Application->doRun()
Application.php:175, Symfony\Component\Console\Application->run()
Runtime.php:110, Drush\Runtime\Runtime->doRun()
Runtime.php:40, Drush\Runtime\Runtime->run()
drush.php:140, include()
drush.php:119, {main}()
It looks like the issue only shows up in Drush, too.
This looks good to me. With the patch applied and the finder re-saved and exported, I get the same dependencies that I manually inserted.
I didn't realise that composer.json files are optional if all your dependences are Drupal modules, so I'll close this.
Ah, so it does exist. Problem is that it doesn't when you enable the module :)
% ddev drush en finders_facets
In RouteProvider.php line 211:
Route "entity.finders_facets_type.collection" does not exist.
Failed to run drush en finders_facets: exit status 1
I'm here.
Adding a couple of dependencies for the content types for my exported finder fixes this issue:
langcode: en
status: true
dependencies:
module:
- finders_events
config:
- node.type.localgov_event
- node.type.localgov_event_channel
id: localgov_events
label: 'LocalGov Events'
type: events
channels:
node:
- localgov_event_channel
entries:
node:
- localgov_event
I get another error later in the install process but I'm not sure if it's related yet.
Ta. That was helpful.
I've figured out that the date_recur field doesn't exist because in Finder::ensureFinderConfig(), getEntryBundles() returns an empty array and then configureAsEntry() isn't called.
Stepping through, it seems that when the entity type is loaded, it's not found. Either this is some cache thing, or the entity type genuinely doesn't exist yet. This is a possibility as that's being created in this same config import.
I'm also getting this exact error when trying to create a finder from a module's install config. The change in my PR doesn't help in that case though, so I'm thinking it might not be the correct fix.
I attended.
And I've fixed up the original PR which somehow ended up with the other commit in it. (It's still listed on the activity page, but it's not in the changes): https://git.drupalcode.org/project/finders/-/merge_requests/1
Ignore that linked MR - I pushed to the wrong fork somehow.
From the core discussion ( https://www.drupal.org/project/drupal/issues/3278431 📌 Use PHP 8 constructor property promotion for existing code Needs work ) I took that using constructor property promotion should be encouraged for new code. LGD's code uses it for most classes already.
Yup, and that's the code that causes the problem by going on to not create the routing for the canonical link.
If you look through EntityBase::toUrl(), that checks to see if there's a canonical link template. If there is, it uses the (missing) route. If there isn't a canonical link template, it falls back to the edit-form link template. I think this behaviour is what we want to happen.
I feel like the pragmatic fix here is to take out canonical from the Finder entity, but there's arguably something that should be done better in core. (Maybe adding Drupal\Core\Entity\EntityViewBuilder as the view_builder for entities that declare a canonical route template and no view_builder?)
The error message is still going to be confusing for a lot of people. IE it still says "Adding non-existent permission(s) to a role is not allowed." Most people in the thread have run into this issue long after the permissions have been added to the role, and were trying to do something completely unrelated to roles or permissions. I think something like "Non-existent permission(s) @permissions have been found on role @label (@id) and have been removed." would be more useful.
The changes look good to me, but when reviewing, I realised that the __create() and construct() methods are now identical to the parent class, so they could just be removed.
(Hi Darren! Been a while...)
The solution from #2 is working for me. I have to rebuild the index after changing the synonyms, but after that it works as expected.
Thanks for the review! Using DateTimeImmutable makes sense, so I've changed that.
I've added unit test coverage for the new code to handle dates in FacetResultParser.
I also made the code look at the ISO 8601 date string that gets returned, instead of using the integer value. This means if for some reason we end up with the date with precision in seconds instead of milliseconds, the facet should still work.
The CI's failing on the next minor composer step, which looks to me like it's opensearch-php's issue to fix. They're discussing here: https://github.com/opensearch-project/opensearch-php/issues/199. (TLDR: opensearch-php via ezimuel/ringphp depends on react/promise ~2.0, composer 2.8 depends on react/promise ^3.2). Everything else passes though, so I think this is ready for a proper review.
Tests fail on code style at the moment (see #3477071). I've made the existing tests pass with the new code, but need to add a test case for the new functionality.
I've also just run into this issue having been upgraded directly from 2.1.3 to 2.2.0, with my composer constraint being "^2.1" for honeypot.
Expecting people to do multiple deploys for a minor release of a module isn't reasonable. Please revert the changes in #3464350 to put the update hooks back.
Personally, I'd be inclined to leave old update hooks in for an entire major release of a module, so people are free to update from any 2.x branch to 3.x.
The $elementInfo property never gets used as far as I can tell. If I'm right, it'd be better to remove it.
@apotek This issue isn't present in the 8.x-1.x branch, so there's nothing to port. You can find the roadmap for a 2.x release here: https://www.drupal.org/project/tfa/issues/3374845 🌱 Roadmap for 2.0.0 release Active
I don't know if this is ready to merge yet - mainly as I have yet to test if I've broken existing functionality with Solr. This MR is working for me with OpenSearch though.
If you'd like to run it, you'll need to be using search_api_opensearch 3.x (which is currently still in dev) with the changes from #3454606 applied.
To explain the changes in this MR:
All the changes to existing code ate in BestBetsProcessor::preprocessSearchQuery(). I've moved getting the QueryHandler plugin instance to its own private method to clean up preprocessSearchQuery(). I've also added a new method to the QueryHandlerPluginInterface to get the search keys from the query and run any preparation on them that the search backend requires. This is because the keys for OpenSearch come back from the Query object as an array of strings, which made the existing code return early at the check on is_scalar(). The OpenSearch QueryHandler plugin therefore reassembles the array of strings back into the original search term. The Solr QueryHandler plugin just returns the search keys as-is.
The new code is the OpenSearch query handler, which adds the items to boost or exclude to the query, and an event subscriber for the OpenSearch QueryParamsEvent, which alters the request made to OpenSearch to do the actual boost.
If one of the maintainers of search_api_best_bets could give this MR a quick review to confirm it's generally acceptable, I'd appreciate it. Once that's done, I'll go back, test for regressions with Solr, etc. Thanks!
Thanks for the review. I've moved the MR and issue to the 3.x branch.
I think the message added by the logger is confusing. I ran into this issue when trying to deploy updates to a site. Somehow a couple of roles had permissions left that no longer existed. (Workflow permissions from a content type that was removed). The updates were trying to add new permissions that did exist to the role, but the extant permissions were triggering this error.
In my case, silently removing the permissions that no longer exist is what I'd like to happen. The decision to remove those permissions has already been made. The fact that they're still assigned to a role is an error, and one that we can easily correct for people. Logging an error saying "Adding non-existent permissions to a role is not allowed." is confusing, as nothing was trying to add non-existent permissions.
I've gone with patch #11 and it's worked well for me.
Excellent - thankyou!
That call to sleep() actually came over in some code I copied from OpenSearchBackendTest. I've swopped it out in both that test and the new SpellCheckTest for a call to refresh the index. The tests both pass, and now they run faster, which is nice.
I've added a kernel test to cover the spell check feature. How's that looking now?
Thanks for the review kim.pepper.
I've added unit tests for the new classes SpellCheckBuilder and SpellCheckResultParser. While doing that I realised that the builder was getting passed data it never used, so I've cut down the params to SpellCheckBuilder::setSpellCheckQuery(). I also found a bug in SpellCheckResultParser::parseSpellCheckResult(), where suggestions that appeared early in the response from OpenSearch would be ignored in favour of later ones with the same score. I've replaced the logic there with code that won't ignore any suggestions, and ranks by both score and frequency to find the best ones.
I'm going to work on a functional test now.
The tests are green now (except for some code style failures in other classes unrelated to this change).
I've tried out the code in this PR locally, and it's working well for me.
Great - I'm happy to add tests for any changes. It's something we aim to do for all the code in LGD too.
The only other change to the module that we're looking at right now is the existing issue #3025156. One of our councils has run into the same issue on two of their sites, and found that the patch you put in that issue resolves the issue on one of them but not the other. I'm going to work with them to find a solution that works for both of them.
Good call. Here's a updated patch with a unit test for the new option.
I've granted you permission. Go for it :)
Uploading a patch that implements a setting to keep existing IDs when filtering.