I've got the dropdowns working, will clean up and push to MR soon.
> Using $this as a pointer for form callback [$this, 'someMethod'] is an antipattern and should be avoided.
Instead of just saying it's bad, we just not allow it and throw an exception for it?
What's the reason it's bad though?
There's been a regression here:
label_count used to be a PluralTranslation, which enforced the keys:
public function __construct(array $values) {
if (!isset($values['singular'])) {
throw new \InvalidArgumentException('Missing "singular" value in the PluralTranslation annotation');
}
if (!isset($values['plural'])) {
throw new \InvalidArgumentException('Missing "plural" value in the PluralTranslation annotation');
}
Now it's just an array and there's no check of the keys:
public readonly array $label_count = [],
Ok, so I am leaning towards:
- My MR for checkboxes & links widgets, done with a processor
- A custom widget for dropdowns
@rupertj what do you think?
Urrrrgh except that Events 3 has multiple dropdowns, one for each taxonomy term. ARGH.
> And it doesn't work with the dropdown widget -- do we need to support grouping in the dropdown?
It occurs to me that using a dropdown here doesn't make sense -- you can only pick 1 facet from the dropdown, and the whole point of Finders facets is that there are several orthogonal facet types. Selecting 'cats' and then not being able to pick 'purple' or 'Italy' makes no sense.
Aha -- I added alter hooks for everything! Hurrah!
Can this be closed in favour of https://github.com/localgovdrupal/localgov_events/issues/199?
@eric_a yes, but what's their reasoning for the difference?
Good catch on the other callbacks!
And nice job making the test a unit test.
But I'm really not keen on the mixing of if/else and ternary that there is now.
We could maybe use the nullsafe operator like this (pseudocode):
$entity = getEntity()
$label = $entity?->label();
// Then we are back in the case where label is either a label or NULL.
> we don't lock the logic into DrupalKernel, which is what the currently proposed MR is doing
The MR is putting the value of the app_root into a class created by the Composer scaffold plugin. That class is considered API -- DrupalKernel can read it, or something else can.
I put the fallback logic that we also need into DrupalKernel, but it could go somewhere else -- where should it go?
> The fn keyword MUST NOT be succeeded by a space.
What's the logic behind this?
It's a bit confusing to have different spacing rules for the short and the long anonymous function notation
I'm not sure what to do with this now.
I had assumed that after 📌 Use a better container cache key Active got in, we'd rework this a bit to add the APP_ROOT constant to the new DrupalInstalled class file that the scaffold plugin now generates.
But the whole of the DrupalInstalled.php file gets written in Plugin::preAutoloadDump(), which isn't where the rest of the file scaffolding happens, and it also means that there's nothing to handle the ScaffoldResult object that ManageDrupalLocations in the MR is returning.
> This would allow the $app_root determining logic to be placed in the new DrupalRuntime class
You're going to need to read the TL, I'm afraid -- there are lots of comments above where I figured out that determining the $app_root at runtime isn't performant.
> One thing to make sure we have here is support for sessions.
Does anyone have any ideas about how to make sessions work?
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.