Thanks for the report and the MR. For a crash, status can be 'critical' :)
Core changed this in 🐛 BreadcrumbBuilder::applies() mismatch with cacheability metadata Active -- it's a pain as it's impacted other contrib module as well as Entity UI.
Committed, and will make a new release now.
(BTW remember to set an issue to 'needs review'.)
joachim → made their first commit to this issue’s fork.
This has broken several contrib modules that inherit from PathBasedBreadcrumbBuilder --
-
🐛
BusinessRulesBreadcrumb must be compatible with PathBasedBreadcrumbBuilder on Drupal 10.4.0 and above
Active
-
🐛
Fatal error: Declaration of Drupal\entity_ui\Breadcrumb\AdminBreadcrumbBuilder::applies
Active
Linked to the other page -- this is a total mess though, needs more clean up.
Also, remove the jovial tone and use of 'easy' -- not appropriate.
-1 from me.
The improvement to readability happens every time a developer looks at the code in question. That's a frequent benefit.
The downside of a merge conflict happens only when that code is changed. That's a rare cost.
Done.
That was possibly done for the typed class constants I'd put in, which I later removed.
Try it on PHP 8.1 and see what happens :)
Though honestly, PHP 8.1 is security releases only, and will be EOL in 10 months' time.
What about the actor not being an entity at all?
My use case is a personal blog site. I want to configure which node types are posts, but I don't need it to be tied to a user account.
There is https://www.drupal.org/project/site_entity → which provides a singleton site entity, but it doesn't have any releases and looks abandoned.
joachim → created an issue.
Uploading a patch on 3.8.x so it can be used on D10.
Devel still shows the tab without the link template, so all good.
Merged.
My hunch about the BaseFieldDefinition check in date_recur was right, and so fixing that to not produce a warning is the simplest fix.
Therefore, I am:
- removing the Views data workaround in this module, which means a views data rebuild causes a warning
- fixing
🐛
views data doesn't work with bundle fields and produces a warning
Active
in date_recur
- adding a note to the README about needing the patch to prevent the warning
LocalGov Events should add the patch to its composer.json -- off to file an issue for that.
Revisiting this, as the workaround I added to Finders Events turns out to cause problems of its own.
This module's views data code in DateRecurViewsHooks is catering for both base fields and config fields.
Given the lack of support for bundle fields in core Views, it shouldn't babysit and try to support bundle fields.
However, it's accidentally treating bundle fields as base fields because of these checks:
> if ($field instanceof BaseFieldDefinition) {
These will fail with modules that use a BundleFieldDefinition class to define bundle fields. This is found in Commerce and also in Entity module -- https://git.drupalcode.org/project/entity/-/blob/8.x-1.x/src/BundleField...
Calling isBaseField() is the right way to check for this instead. MR coming.
Ultimately, we don't massively care about these fields getting declared to Views, since our Views support goes via SearchAPI.
Our options are:
- fix the bugs in core. Not easy to do -- this touches on
🐛
[PP-1] bundleFieldDefinitions() are not added in EntityViewsData
Needs work
and
📌
Allow @FieldType to customize views data
Needs work
- fix the bug in date_recur. It's getting *all* field definitions and passing them on to a system which expects only config fields. I suspect the checks for BaseFieldDefinition are the problem
- decorate DateRecurViewsHooks to wrap getDateRecurFields() and remove the bundle field
My comment at the time says:
// Declare our event bundle field to Views ourselves, as core doesn't
// declare bundle fields to Views, and Date Recur module's Views support
// complains if it's not there.
and without the hack, we get this error instead:
> Warning: foreach() argument must be of type array|object, null given in Drupal\date_recur\DateRecurViewsHooks->viewsDataAlter() (line 191 of modules/contrib/date_recur/src/DateRecurViewsHooks.php).
So, the problem is that one the one hand, DateRecurViewsHooks::getDateRecurFields() loads all date_recur fields using the field definition API. That gets you code AND config fields.
But views_field_default_views_data() is working with config fields only.
The two areas of code are disagreeing about what they're doing.
NM, my breakpoint fired when I loaded the finder edit form.
In the backtrace we have date_recur_field_views_data().
That calls datetime_range_field_views_data(), because date_recur fields sort of inherit from datetime_range fields.
We end up in datetime_type_field_views_data_helper().
That calls views_field_default_views_data().
This is all for the field finders_events_date -- a bundle field!!
Which has me wondering how the hell a field storage config entity is there for a bundle field that's in code.
Aaaaand... the culprit is... Finders Events:
// Invoke hook_field_views_data() directly on date_recur module with our
// dummy field storage config entity, so we can use that as the views data
// for our bundle field.
$data = $module_handler->invoke('date_recur', 'field_views_data', [$dummy_field_storage_config]);
It's me being too clever. Not sure how tests didn't pick this up!!
I only get this error when I first install the module.
Could you give me steps to reproduce it without doing that? I've tried deleting the view and then importing earlier-exported config and there's no error with that.
It was a stupid typo :(
Can this be closed as fixed?
> % ddev drush en finders_facets
Is that with facets and finders already enabled?
> What you're actually meant to do with those (Help - Permissions - Configure) "links" that appear in the terminal, I don't know.
Are they clickable links? Drush is printing them with href tags:
> return sprintf('
%s', $link->getUrl()->setAbsolute()->toString(), $link->getText());
public function getViewIds(IndexInterface $search_index): array;
public function getViewIds(IndexInterface $index): array;
@ekes I prefer to be explicit and call it $search_index, because $index often means a numeric index of an array!
What's the PHPUnit warning that you mention in the message for commit 804d6b0d39ed15b29fb33aea92d173878635d7b6?
Ok so it's not just that an entity collection route isn't ready at that point -- I found LOTS of contrib modules with this, e.g.
eca/modules/ui/eca_ui.info.yml
6:configure: entity.eca.collection
facets/facets.info.yml
6:configure: entity.facets_facet.collection
geo_entity/geo_entity.info.yml
14:configure: entity.geo_entity_type.collection
metatag/metatag.info.yml
7:configure: entity.metatag_defaults.collection
What was the drush command that caused this, and what did you have enabled prior to it?
And what version of Drush is it?
Thanks!
There's a ticky box next to the 'Create fork' button that says ' with new branch from ...'. If that's not selected, it won't create a new branch.
It might also be that you were too quick for it and the new branch hadn't been made yet. Or that there was a glitch in the process.
@rupertj Could you try this MR please?
Do you mean when you look at a facet type, there's a tab for manage display?
It's added by Drupal\field_ui\Routing\RouteSubscriber to any entity type that's fieldable. We could hack it out by registering our own route subscriber that runs after field_ui's one, but is it worth the effort?
Thanks!
Looks great - RTBC!
Perfect, thanks!
Looks good, though the first line of the doc block is now too long. We need to shorten it. I'd say the word 'available' isn't adding any meaning, so we can remove that.
Remember to set an issue to 'needs review' when it's ready.
(I somehow managed to copy-paste the interface name wrong though...)
Argh.
Which RouteProvider.php file is that?
This changes the path alias processor to handle path aliases as entities.
The fact that the core path alias field sort of and sort of isn't a reference field makes it a bit more complicated than the entity reference processor.
I've added a base class for kernel tests too.
If a custom field type is putting an array into $field_data['value'], then presumably it's using the 'map' data type for that property?
I don't think we should be handling an array like that -- rather this code should be stricter about detecting rich text fields:
> if (isset($value['format']) && isset($value['value'])) {
The answer will be in SearchAPI -- it calls getItemIds() on a datasource. Does it handle the scenario where an ID it got last time it called that is no longer in the returned list?
Isn't that the route that will get defined automatically by this in the finders_facet_type entity type?
> 48: * "collection" = "/admin/structure/finders_facet_type",
I wonder why this is only complaining for finders_events_date, and not for the other bundle fields the Finder is defining.
foreach ($bundles_names as $bundle) {
$fields[$bundle] = FieldConfig::loadByName($entity_type->id(), $bundle, $field_name);
}
This looks like Views getting tripped up by bundle fields.
I have to say, I probably didn't check my local dev copy for log errors!
(Another reason why we need Kernel tests to fail on log errors!)
Maybe need an issue at date_recur_search_api to do the same to Drupal\date_recur_search_api\Plugin\search_api\datasource\DateRecur
.
That's promising!
The Finder entity class needs to implement calculateDependencies() to declare these.
> I believe this is a significant change that could cause WSODs after an update. Instead of throwing an exception, we should trigger a deprecation error first.
Agreed, and if that's now the case, then this in the CR is wrong:
> The former will no longer be permitted. An exception will now be thrown if attempted.
I can't remember how much work I did on declaring config dependencies, but it's quite likely I was a bit lax about it. If those are in place, Drupal's config install system will know what to install in what order.
On the other hand, ✨ 3rd party should be allowed to add config dependencies Active aaaargh.
I think this is ready.
BTW @vladimiraus it's much clearer to rebase on 11.x rather than merge it in, as that way you don't get merge commits in the feature branch's history.
> Drupal\search_api\SearchApiException: The "date_recur:node__finders_events_date" plugin does not exist. Valid plugin IDs for Drupal\search_api\Datasource\DatasourcePluginManager
I think it might help to draw up a hierarchy of plugin dependencies.
The datasource is derived from the existence of the date_recur field which IIRC the events Finder type plugin will have defined.
Can you debug in DateRecurDatasourceDeriver to see why it's not seen at that point?
There was a README file in the D7 version which had most of its content removed for the D8 port! I don't know how that happened!
That should be restored as an initial step, if it's still accurate.
@prem suthar I would wait and see what the maintainer thinks of this change before working on this.
Also, I'm making changes that will clash with this in another issue!
@chandansha I would wait and see what the maintainer thinks of this change before working on this.
Also, I'm making changes that will clash with this in another issue!
joachim → created an issue.
There's actually another, more important reason why this can't be a plain reference field: it works backwards compared to normal reference fields.
With a term reference, say, you do this when saving new entities:
1. Save the referenced term, so you get an ID for it
2. Set the ID as the reference field value
3. Save the referring node
But with path alias fields, it's the other way round. We have to have the ID of the referring node in order to get the system path to set in the path alias entity. So:
1. Save the referring node
2. Get its canonical path, /node/X
3. Save the path alias, with the canonical path and the alias given in the node's values
Done.
Will it be able to work exactly like an entity reference field?
At the moment, the way the API work is that you can save entity data like this:
'title' => 'Some test data',
'type' => 'alpha',
'path' => ['alias' => '/one'],
and the path will get saved.
So it's not just a case of handling the path alias field widget differently -- the path alias field does magic at the API level too.
It should definitely behave like a *type* of entity field -- use EntityReferenceItemInterface/EntityReferenceItemBase and EntityReferenceFieldItemListInterface for one thing, and add the 'entity' computed property. But I think it will still to still be its own kind of reference field.
Your new MR is missing all the changes from the original MR, such as removing the calls to this method in core!
Please can you make the new deprecation message changes on the *original* branch and MR, and close the new branch and MR.
A new branch/MR is only needed when we are trying a completely different approach. That is not the case here.
Made a MR with patch #23.
Still needs work for various comments above.
It would still be really useful and a DX improvement if all entities, whether config or content, had the same API for validation.
> What about just overriding label() on the entity types that don't have a label and return something useful?
Views modes and fields have labels already though.
And in any case, overriding label() would be too broad.
This is a specific place where more context is needed. In most instances, such as an admin list, or a message, there's no need for the additional context.
Overall looks good, but the deprecation docs and error messages need to follow the standard format at https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → .
Thanks!
I've rewritten it a bit as it was mostly the text of the issue summary. The CR has a different audience from the IS -- the IS says what's wrong and what we're going to fix and how, but the CR needs to tell people what's changed and how to update their code in response. If people reading the CR want the background, they can follow the link to the issue.
There isn't a Change Record yet. Use the link in the issue details to create a draft.
We don't need a new merge request or a new branch -- you can keep working on the existing one.
We should define a standard for where plugin base classes and interfaces go.
Having these in the plugin folder is good DX. Having them dumped in top-level /src is messy.
I think @catch suggested /src/Plugin in slack?
> I wonder whether we could/should give a deprecation notice to classes that have that method, or whether that's overkill.
Spoke to @longwave in Slack who says we don't need to do this.
We need to restore getFormName() in the base class, and instead, add to it a deprecation notice so that any calling code gets notified to stop calling it.
We should keep the removal of getFormName() in the interface though, as we want implementations to remove it.
I wonder whether we could/should give a deprecation notice to classes that have that method, or whether that's overkill.
We need
Resolved 2 comments. I think the 3rd can be left as is.
This all sounds good to me! :)
Also, having a __construct in an interface is wrong.
Rebased, but there is a change in core/tests/Drupal/Tests/Core/Http/ClientFactoryTest.php that doesn't look necessary to me. We shouldn't need to change any tests!
Removing pointless tags.
> Looking for reviews/opinions on whether adding autowire as a boolean property to every plugin definition is the correct solution here.
Dumping it in the plugin definition seems wrong, when it's something that's only needed internally, and not by any code using the plugin definition.
Can this be closed as a duplicate of ✨ Add drupalGet() to KernelTestBase Active ?
Resolved everything and rebased.
I attended.
> 1. Artificially change the plugin on a Views field in Views data, and check that a view using that field doesn't pick up on it
On reflection, this is probably easier.
1. Make a view with a field, say on the node title
2. Create and enable a custom module which:
-- has a Views field handler plugin which always outputs 'banana'
-- implements view data alter hook which sets this plugin for the node title field
3. Existing view should now show 'banana', but it still uses the original plugin
Shouldn't these be long docblocks, the same as class properties?
enum Exists {
/**
* Throw an exception when the entity exists.
*/
case ErrorIfExists;
Unless we need non-Drupal packages, I tend to think that a composer.json is just a maintenance burden -- it duplicates what's already in the info.yml file.
There isn't an exact replacement, and in any case, that's information about Drupal's APIs -- it doesn't belong in the coding standards.