Thanks for adding the steps to reproduce. Though you can probably do it more quickly by using one of the test entities in entity_test module, and changing one of its handler to a class name that doesn't exist.
I'm trying an alternative approach using a Facets processor.
Sadly, I don't think I can do everything in the processor -- it still has to do a bit of hacking at the theme level.
And it doesn't work with the dropdown widget -- do we need to support grouping in the dropdown? We could provide a custom widget for that if it's needed.
Another alternative approach to the two so far would be to provide parallel versions of all the major widgets -- checkboxes, links, dropdowns. But that seems quite heavy.
I'm splitting off the removal of out-of-channel facets to ✨ add a facets processor to filter out facets not in the current channel Active as that definitely CAN be done in a processor.
@rupertj what do you think?
Ahhh right it's twig for Views to interpret. Sorry, I got completely mixed up and thought it was you thinking that twig would be handled when the config is created from the template file.
We can't use the LGD twig filter here, as it's not a dependency.
Hardcoding it to the same value would be doable, but not ideal. I'll have a look at whether there's an alter hook.
I don't think that's going to work.
It's been a while since I've looked at the code, but I don't think the config templates use twig -- they use a variety of replacement techniques, each one slightly difference IIRC, as I was gradually improving the DX for each one.
I've installed everything on a fresh(ish) site, and I can create event nodes fine.
Is this MR still needed?
This works to put the links in the render array:
$render['#view_id'] = $view_id;
$render['#view_display_show_admin_links'] = TRUE;
$render['#view_display_plugin_id'] = 'embed'; // WTF ARRRGH THIS GETS 'default' WTF $view->getDisplay()->getPluginId();
views_add_contextual_links($render, 'view', $display_id);
but they are not output. Possibly we're removing things, or not outputting the right things in twig?
This is going to be fiddly.
But I think it's doable.
We refactor the code to first collect channel entities, and then have this block only once:
foreach ($channel_entity->get($facet_types_enabled_field_name)->referencedEntities() as $facet_type) {
That means we can distinguish between:
a. there are no channel entities to look in
b. no channel entity has any facets enabled
I'm still not keen, but insisting it goes in the LGD module would make things really faffy. I'm outvoted :)
Confirming that EventFinderFacetsPluginTest.php passes with this MR.
Thanks!
I will not look at or consider code that has been generated with AI.
Thanks for the explanation @klausi. That sort of thing would be useful to have in code comments!
The other way would be to store the revision ID of the latest revision somewhere.
> I think the right thing to do here is to revert the change
In the interests of fixing the performance regression, yes.
But there should be a follow-up issue to remove the subquery entirely, which will improve performance further.
I've installed the demo, and I'm not any clearer on why there are unit types AND unit bundles. And now I'm also confused why there is a unit entity type, but a node type for rooms. Isn't the unit entity the room?
Thanks for the link - will give it a try.
Are you sure you mean this issue for testing instructions? It just says to add a README. If its scope is meant to be more specific, it should maybe be renamed.
Couldn't this be done with a self-join instead of a MAX subquery?
> You can certainly mock, or decorate, the class resolver to return a faked version of the config importer.
Mocking the whole class resolver feels rather heavy!
What's the reason for moving away from the factory service there was earlier?
> I'm not sure what you mean here -- why wouldn't we be able to mock it in tests?
Things that use a ConfigImporter are hardcoding the class name:
$config_importer = $this->classResolver->getInstanceFromDefinition(ConfigImporter::class)
->setStorageComparer($storage_comparer);
With the factory service in the earlier versions of this MR, you could if you needed to mock the factory service with something that returns a mocked ConfigImporter.
I can confirm the MR fixes my problem with the duration field.
See https://www.drupal.org/project/drupal/issues/3477380 ✨ add a plugin type ID Active .
Also, what does a Booking entity represent? It doesn't seem to have fields that reference anything else.
I assumed it was the thing that was an actual booking of a unit? How does it work?
> make it much easier to instantiate: \Drupal::classResolver(ConfigImporter::class)->withStorageComparer($storage_comparer).
But if we have that, how can it be mocked in tests?
I don't see an answer about Unit types and Unit bundles over there.
The current MR makes loads of changes to files, and there's no changes to the main README.
Latest patch doesn't apply. Made an MR with the same change.
> Not confident on my own at this layer to make that decision - kind of needs a maintainer to feed in
This isn't the issue to fix it -- this is meant to be a refactoring & optimising issue.
As you've been working a lot with this code, you're in a good position to say whether we should fix that before or after this issue gets in.
It would be really useful if the README also gave an overview of the architecture of this module, and how it all fits together.
For example, I can't figure out at all why there are Units and Unit bundles, and then a Unit Type content entity too, which itself has a Unit Type bundle!
> @trigger_error('Passing a Condition to \Drupal\Core\Entity\Query\Sql\Query::condition() that was generated by a different query is deprecated in drupal:12.0.0 and removed in drupal:13.0.0. See https://www.drupal.org/project/drupal/issues/2875033 📌 Optimize joins and table selection in SQL entity query implementation Needs work ', E_USER_DEPRECATED);
Is this removing the behaviour that was added in https://www.drupal.org/node/2770421 → ?
The andConditionGroup behaviour in #71 sounds like a bug to me.
The way conditions are documented, shouldn't both versions of the code return the same thing?
> 'There was a problem creating field:
That doesn't make grammatical sense -- if there's no label to use, it should say something like 'the field'.
Diátaxis looks like a good way of thinking about docs.
> Diátaxis solves problems related to documentation content (what to write), style (how to write it) and architecture (how to organise it).
The 'how to organise it' part is going to be the tricky bit, I think -- as in where do we put what sort of docs.
This needs to be made as a MR.
BTW, out of scope here, but the docs for addEntityJsonapiUrl are total crap.
* @param array $matches
* The link information.
It's not -- it's the matches from the preg_replace_callback(), which is why we return $matches[0] -- that is, the entire matched string -- if we don't want to act.
The DX I was thinking of was this:
// I know this might log some errors, but they are not logged inside a try{} block.
$this->expectNoLogErrors();
$sut->doSomething();
// I know this might log some errors, but the SUT is wrapped in a try{} block.
$sut->doSomething();
$this->assertNoCaughtLogErrors();
Having two different ways to check for this is less good that the approach we were working on previously, but the advantage is that the reporting from PHPUnit is consistent and correct.
Making the region available to block templates should really be done in the block system, so it's not theme-dependent.
There's no need to change field_tools.admin.inc -- it's dead code. I left it in out of superstition that I've not yet converted everything that was in there to OO code.
Thanks for working on this!
Left a comment on the MR.
I'm surprised by that! I implement ServiceModifierInterface in tests a LOT, to do things such as swap out EntityTypeManager to then fiddle with entity type definitions.
As we change more tests to be Kernel tests instead of Functional, I think we'll need to use ServiceModifierInterface more often.
I'm not convinced of the need for a trait for this at all.
EntityViewsData is incredibly crusty and hasn't had much change in years, and a cursory search shows that this code is still here:
if ($revisionable) {
$revision_table = $this->entityType->getRevisionTable() ?: $this->entityType->id() . '_revision';
}
It means that tests that need more altering will need to fiddle with the trait to alias its alter() method.
I think it's a simpler and clearer pattern if the trait supplies a non-interface method alterTimeService(), and all services implement alter() to call it.
The alter() should not be in a trait. Tests may need to perform other alterations to the service container.
> However, it seems like it would do a bad job at handling multiple logged errors, because we could only rethrow one.
I think that's a good thing! The first one fails the test, you fix it, then you get another one, and you fix that one. This is the same way that test failures work -- you only see the first one.
I'm concerned there could be other places where migrate messages are misused, but let's fix this as a novice issue.
Found it:
// A shared table contains rows for entities where the field is empty
// (since other fields stored in the same table might not be empty), thus
// the only columns that can be 'not null' are those for required
// properties of required fields. For now, we only hardcode 'not null' to a
// few "entity keys", in order to keep their indexes optimized.
// @todo Fix this in https://www.drupal.org/node/2841291.
$not_null_keys = $this->entityType->getKeys();
// Label and the 'revision_translation_affected' fields are not necessarily
// required.
unset($not_null_keys['label'], $not_null_keys['revision_translation_affected']);
// Because entity ID and revision ID are both serial fields in the base and
// revision table respectively, the revision ID is not known yet, when
// inserting data into the base table. Instead the revision ID in the base
// table is updated after the data has been inserted into the revision
// table. For this reason the revision ID field cannot be marked as NOT
// NULL.
if ($table_name == $base_table) {
unset($not_null_keys['revision']);
}
Urgghh. Spooky side-effects FTL :(
We probably need to add 'changed' to the keys that are unset here.
And this is a spooky side-effect of entity keys which AFAIK isn't documented at all!
Totally tripped us up on 🐛 entities implementing EntityChangedInterface should have a 'changed' entity key Active .
Ok I can confirm that with the MR, the 'changed' field on taxonomy terms doesn't get an ALLOW NULL, and without the MR, it does.
I have NO idea how!!!
I can't reproduce the test failure locally -- this gets in the way: #3552984: MigrateTestBase::display() causes a TypeError if a test fails a migration → .
> Unless our custom endpoint includes the JSON:API data, in which case we're no longer using vanilla JSON:API at all!
And if we do that, then I would seriously consider moving away from bundle channels, as having to make a channel for every bundle is a pain. Bundle-based endpoints is part of Drupal JSONAPI's opinionated design, which works for using JSONAPI in general, but isn't really useful or helpful for Entity Share.
Wow, that's some impressive test failures!
AFAIK nothing is looking for the 'changed' entity key, since, again AFAIK, we're inventing it in this MR. And yet, things fall over dramatically:
> ├ SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'changed' cannot be null: INSERT INTO "test36480953taxonomy_term_field_data" ("tid", "revision_id", "vid", "langcode", "status", "name", "description__value", "description__format", "weight", "changed", "default_langcode", "revision_translation_affected") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11); Array
@mradcliffe I don't understand what the IS is missing.
> We should look at other core implementations of the method to find other exceptions that they might throw, and write documentation that covers those reasons.
What else needs to be said?
The text at the top of https://www.drupal.org/docs/develop/standards/obsolete-php/obsolete-api-... →
> The standards have moved to GitLab pages, Drupal coding standards.
links to the wrong place: https://project.pages.drupalcode.org/coding_standards/composer/package-n...
> This seems as desired? I assume that the various other tests edited in the MR were edited to fix them after they were broken by the MR?
Yup. The tests have been updated to work with the new factory service, so running tests without the new code will fail.
I might have a better approach for #108.
It's possible to store an exception and throw it later:
<?php
function sut() {
throw new \Exception();
}
class Catcher {
protected $e;
function catching() {
try {
sut();
}
catch (\Exception $e) {
$this->e = $e;
}
}
function check() {
throw $this->e;
}
}
$c = new Catcher();
$c->catching();
$c->check();
This fails on the thrown exception in check(), but the error reported makes it look like it happened in catching().
This means that our logger could throw an exception at the point it wants to say things have gone wrong, but catch and store it, and then when asked by the test whether everything is OK, throw that exception.
You'd then get the backtrace of what actually caused the problem, which otherwise could have been swallowed up by something like the batch system or cron.
Do we really need tests for this? It's going to need an entire test module with bad config just for this test.
Nitpicking my own decisions: should it be Mock singular rather than Mocks plural in the namespace?
Please don't use AI to summarise the issue. It just adds a huge amount of noise and nothing useful.
There's no need for a test for documentation.
Are you using an AI to write your comments? Your comment reads quite a bit like what an AI bot would say. I'd rather not interact with AI, and I advice against using it to make the patch.
> Also for JSON:API Extras field enhancer plugins, no more need to alter JSON:API output to provide data for the client to search for referenced blocks
It's true, we could totally ditch the need for JSON:API enhancers.
But then that would be a big change in the philosophy of the module, that it uses JSON:API as its transfer method. We'd be saying that we use JSON:API for the content, but custom endpoints for relationships.
It would then mean twice as many requests, since for each entity we'd need to call first the custom relationships endpoint, and then the JSON:API endpoint for the content. Unless our custom endpoint includes the JSON:API data, in which case we're no longer using vanilla JSON:API at all!
> Server website will have already prepared the JSON:API urls to search directly.
The server would need plugins to do that for the various things -- books, embedded, aliases, etc etc. So we'd either need a new family of plugins, or use processor plugins both server- and client-side. In either case we'd need some sort of config on the server that mirrors the import config on the client.
> $this->handlers = [];
There's nothing else in EntityTypeManager which does this, so still needed.
> Mind blown - I must have missed this in the instructions
:)
Are there places where extra instructions could be added, or made clearer? Please file a docs issue if so!
Is the MR still needed?
> In import config I have this check box selected "Embedded entity"
You also need to enable the JSONAPI enhancer for that field.
I don't think it's a good idea for a tests issue to be making changes in non-test code as well. Those probably need their own issues.
> AdminToolbarToolsConstants
It looks like this is only used for tests? It makes more sense to put this trait in tests/src, or in a test base class.
Also, what are the functional tests checking? Could this be done with Kernel tests instead? We presumably already have Functional tests that test the menu works correctly. Here we just care that certain conditions produce certain menu item plugins.
What would be the benefit of gathering dependencies on the server rather than on the client?