Left a comment on the MR.
I'm not actually sure how we fix this, as the dual meaning of the term 'config prefix' probably spreads to other things. e.g. in the storage handler we have:
* The prefix consists of the config prefix from the entity type plus a dot
* for separating from the ID.
This bit will need changing too:
> In the entity type annotation
LGTM, though maintainer will confirm this is correct.
Looks good. Thanks!
This is a good start, but they're not fields.
Something like:
> Provides default values for menu link plugin definitions.
I've been informed on twitter by @chx that it's the docs which are wrong.
These need to be changed to say:
- it's optional
- what happens if you omit it
Hook documentation needs to be converted to OO as well before we do this.
joachim → created an issue.
I suspect this is long since fixed, as node type for instance uses this base class:
> class NodeType extends ConfigEntityBundleBase
Because of energy costs of generative AI and its role in worsening climate change, I consider its use to be unethical. I'm completely opposed to adding generative AI to any of my projects.
> (which I've bikeshedded myself again to be name PluginProperty
What about ThirdPartyProperties, to match how the config system calls this concept?
> In order to annotate a plugin with PluginProperty attributes that such apply only when a module is installed, the provider property of the PluginProperty attribute must be set to name of the module dependency.
That makes sense.
It's a shame though, as the huge benefit of attributes is that you get IDE support for property names and types.
Thinking about it, this filecache problem is not just a Drupal problem, it's a PHP problem.
Consider we have package A, which has annotated classes, and it has a class Foo with an annotation for package B which is not a dependency. When the project owner installed package B, the annotation on class Foo is not picked up. Same problem as with modules, just with Composer packages instead. Is it something that's known about upstream?
> The thing with the third party discovery added here is is that it assumes that the plugin definition is a array-dumping-ground.
Yup. I considered having values from 3rd party attributes go into something like a $definition['third-party'][$attribute_provider] array, but I wasn't sure whether that was over-complicating.
Do you think that would be cleaner?
This issue is a contrib blocker though, now I think of it.
Suppose you have a contrib module which invents the 'floopiness' property on core's Block plugins.
If you want to add that to existing plugins, nothing changes -- you use the alter hook.
But if you provided a Block plugin, up until now you could do this in an annotation
@Block {
id = "My block"
floopiness = 42
}
With attributes, that will crash, and your module needs to implement the alter hook for its own plugins.
I think #18 would need a different issue. It's not just the changes to how we assemble data in the discovery class, but also we need a new way for the plugin type manager to say that there are multiple 'official' attributes. And then there'll be tons of bikeshed about how to slice up the entity type attribute...
Looks good!
I've made some comments on the MR.
Needs tests too.
> Considering lumping "bundle" into the work that needs to be done for "base" fields.
That's going to be complicated, because the way this module declares views data is:
1. Rely on hook_field_views_data() to declare the field data -- but hook_field_views_data() is only invoked for config fields
2. Build on top of the hook_field_views_data() stuff in hook_field_views_data_alter()
Hence the warning -- hook_field_views_data_alter() doesn't find what it expects.
Adding the basic data for bundle fields is babysitting the core issue 🐛 [PP-1] bundleFieldDefinitions() are not added in EntityViewsData Needs work .
I think the best thing here would be to add an isset() check and not assume $data[$fieldTable] is present.
> I assume this is what you mean by "bundle fields" == "base fields".
No, bundle fields are fields which are defined in code but are not base fields. I wrote an overview here: http://www.noreiko.com/blog/defining-bundle-fields-code
@plopesc I don't think that would happen:
- Navigation module provides an attribute class NavigationBlock.
- MyModule adds a NavigationBlock attribute to its block plugins, in addition to core's Block attribute
- If MyModule is enabled but Navigation is not, the NavigationBlock attribute is simply ignored.
I got this crash because I had the 'seven' theme installed on my site (but not active).
Patch fixes the crash, though `drush cr` now keeps saying:
> [warning] Theme seven not found.
and I don't know how to make that go away.
Are there any particular parts of the module you're interested in working on?
Bear in mind that MB is just a UI -- the code generation stuff happens in DCB.
This needs an IS update for the title at least, as on D11 it's surely not that function any more!
I found an explanation of 'using filesort' on stack overflow, which says:
> When you don't see "using filesort," it means the order you requested for the query result matches the natural order in which the data was read, so there was no extra work needed to sort the result.
So I'm curious about this. Field values are surely inserted by delta order per-entity ID, so I assume that the reason we're getting a 'using filesort' is because we're loading multiple entities.
What if we did 'order by ID, delta'? That would then probably match the read order.
> Installing a module is always a special case and all bets are off.
Ok, in which case I'll close this as 'works as designed' and I'll add something to the docs on kernel tests.
> The purpose of this trait is mostly to help with web tests and saving entities in a different process
That's not the case. This trait is also very useful in kernel tests, where you call something that will cause an entity to be saved, and you want to test the changes are correct.
Oh. I was looking at the actual return type... :/
Ok in that case setting back to needs review.
Maintainers: add credit to @berdir please, for help on slack.
Seems to need some very aggressive cache clearing AND we have to get the relevant services from the Drupal object and NOT $this->container, because the ones from $this->container are not the same, WTF???
\Drupal::service('entity_type.manager')->clearCachedDefinitions();
\Drupal::service('config.factory')->clearStaticCache();
\Drupal::service('config.factory')->reset();
$storage = \Drupal::service('entity_type.manager')->getStorage($entity->getEntityTypeId());
My plan with this issue -- and I see it's not stated above, but I do remember discussing it with people, probably @longwave? -- was to add the functionality in this issue, and then in a follow-up do refactoring for shared code with browser tests.
I think I'd rather keep the two drupalGet()s separate as there are things in one that aren't relevant at all in the other -- for example, every request calling isTestUsingGuzzleClient() makes no sense in a kernel test.
However, if you think HttpKernelHelperTrait is a better name than HttpKernelUiHelperTrait, to match up with future refactoring, then let's change that. I'm not sure about 'Ui' in a trait about making requests either.
I'm not sure about the latest commit --
- protected function drupalGet($path, array $options = [], array $headers = []): void {
+ protected function drupalGet($path, array $options = [], array $headers = []): string {
The browser test drupalGet() doesn't return anything, so why should the kernel test version?
Latest version of the MR fixes the problem I've been seeing.
The t() issues seem to be fixed too.
Argh I mistyped, sorry. I meant to say:
Declaring dynamic permissions provider classes in the same location as the fixed permissions is good DX.
So in other words, I think a dynamic service provider class should be declared in services.yml, and a dynamic permissions provider class should be remain as being declared in permissions.yml.
If you're looking for services or permissions, those files are where you go to look.
I think this is a change in the wrong direction. Declaring dynamic service provider classes in the same location as the fixed services is good DX.
This is a pattern we should be extending -- #2910814: deprecate magic ServiceProvider file discovery; declare in services.yml → -- not removing.
Looks good. Thanks!
> Apologies, was waiting for the pipeline to pass/fail before changing and requesting code review on this!
That's a fair point!
> Is it When i start working on an issue, i assign to myself and leave as active? Or change to needs work?
You can assign to yourself when you start work on it, as a precaution to avoid someone else working on it too.
'Needs work' is usually set by a reviewer, or by a test failure. Though if you implemented a partial fix and had to stop, you could unassign yourself and set it to 'needs work'.
MR looks good, setting to RTBC!
The @return needs changing too IIRC.
BTW remember to update the issue status when you make a MR.
Looks good!
joachim → created an issue.
The MR fixes the crash with the test case from my last comment.
I'm not sure where a test would go -- struggling to figure out the structure of the existing tests.
The problem is not cache tags, it's cache context.
After one visit to a node edit form, my cache_render table has an entry with this CID:
> entity_view:block:gin_breadcrumbs:[languages:language_content]=en:[languages:language_interface]=en:[route.book_navigation]=book.none:[theme]=gin:[user.permissions]=is-admin
So the next page -- whatever it is -- will get that same cache entry.
I can reproduce this with a numeric field.
1. Create a node type
2. Add a numeric field to it
3. Create three nodes, one of which has an empty value for that field
4. Create a view with fields title, numeric field.
5. Set aggregation to:
- group and compress title field
- SUM on numeric field
Since upgrading to 10.3 I get this when doing 'drush cr':
> Circular reference detected for service "logger.channel.default", path: "asset.js.collection_optimizer -> asset.js.optimizer -> logger.channel.default -> logger.factory -> logger.filelog -> token -> cache_tags.invalidator -> plugin.manager.block".
It's a duration field from https://www.drupal.org/project/duration_field →
I think the comment here is wrong -- lots of field types have a 'value' property:
// Get the commerce value.
elseif (isset($field->value)) {
$value = $field->value;
}
smustgrave → credited joachim → .
Seeing this problem too.
elseif (isset($field->value)) {
$value = $field->value;
}
This gets an empty array.
if ($compressed && is_array($value)) {
$value = reset($value);
if (is_array($value)) {
$value = reset($value);
}
}
reset([]) is FALSE, hence the bad return value.
My view is a list of timeslots which have a duration field, and I am aggregating the duration.
If a timeslot is current, it has no end time and no duration, hence the empty field value.
> @joachim: Did the update function work properly? I can't reproduce this problem, and as I said I believe the update function should be updating any views_aggregator table views on your site.
I'm embarassed to report I hadn't run the update function. Running it fixes the crash -- the update function correctly changes the type of the group_aggregation_results property:
- group_aggregation_results: '0'
+ group_aggregation_results: 0
However, the update function also made these changes which I'm confused about:
- totals_per_page: 1
- precision: 2
+ totals_per_page: 0
+ precision: 0
Rerolled.
joelpittet → credited joachim → .
Tested the patch and it's working well.
The version needs updating, but apart from that looks good.
Looks good, thanks!
Looks good!
I'm using Firefox on MacOS.
Just tried on Safari and that doesn't have the problem.
joachim → created an issue.
Ok, re-saving the view fixes the crash, so it *is* a config update that's needed.
The views preview crash is still happening, so that must be something else.
I was thinking what would be needed here would be to do an update of existing config, but I've just tried creating a new view from scratch, and the preview crashes.
joachim → created an issue.
It's been a week with no objections to either the plan or the name, so I've moved the code for making requests to a trait.
> Which sounds like undefined severities default to REQUIREMENT_OK while installing, and REQUIREMENT_INFO otherwise.
Yup, well spotted!
(Also, the logic in that piece of code could be a lot clearer - I'll file an issue to refactor it.)
(Also, it seems like REQUIREMENT_INFO might only apply during runtime? Again, something for a separate issue.)
I've made a new release of Drupal Code Builder, which adds support for generating OO hooks. It can generate legacy hook support as well.
Great to see this get in. Thanks to everyone involved!
Could you debug and see what's going on at line 1180 please?
I would do this with a computed field.
No, values are computed on load/display, and cached with the entity.
> It would be more straightforward to directly fire an ECA custom event from the action link, so we can specifically react to that in an ECA model, without writing to & reading from the database... Would that be possible?
Yes, that's what I meant. Sorry, I know very little about how ECA works.
You'd probably want a State Action plugin which lets you choose which ECA event to fire.
MR needs a small tweak, but is nearly there!
> Alternatively we could admit the \Symfony\Component\HttpKernel\HttpKernel doesn't work for us and we could copy and maintain the code in a way that does.
The fact that we use the Symfony HttpKernel is pretty useful over at 📌 Add drupalGet() to KernelTestBase Needs review .
There's also the problem of disagreements between the order of \ and _.
e.g. in WizardPluginBase:
use Drupal\views\Views;
use Drupal\views_ui\ViewUI;
Thanks for the MR! Though remember to change the status to Needs Review when you want it looked at.
I've left comments on the MR for changes.
Sorry it's taken me this long to look at this.
Reproduced the bug, confirmed the fix.
Will make a new release.
Thanks!
The new kernel test looks ok, but the whole of the functional test class is being removed, which is losing other tests!
Rebased to 11.x and made a new branch.
I'm removing commit e7790601 because it's making lots of unrelated changes, which is making the branch harder to rebase.
> So for that it would be nice to have these in a trait, and not necessarily with the words "kernel test" in it.
We need something in the name to differentiate it from UiHelperTrait which is for browser tests. HttpKernelUiHelperTrait?
I don't understand what this MR is doing.
> Deprecated function: Calling get_class
That has already been fixed.
33 Access to an undefined property
Drupal\Tests\field\Kernel\FieldCrudTest::$entityTypeManager.
Not sure how to deal with this, as it predates this MR.
Thanks!!
I'm pondering whether to move the methods added to KernelTestBase to a trait, where I'd also add a helper method for setting the active theme.
Thoughts on doing this (and also on what to call the trait? I'm thinking KernelTestUiHelperTrait)
> Technically it is still input type="search". with some extra attributes
With extra *functionality*.
Same way we have an abstraction for type=radios, checkbox and so on.
This is causing problems when new code adds an existing trait, where the trait's methods don't have return types. e.g. 📌 Add drupalGet() to KernelTestBase Needs review comment #38.
Line tests/Drupal/Tests/BrowserHtmlDebugTrait.php (in context of class
Drupal\KernelTests\KernelTestBase)
That seems like a false negative to me -- I can't change the signatures of methods in a trait that other classes are using too.
Why doesn't the response from the HTTP kernel have cache headers? Are those added later in a normal request? And how -- is there a way we can get them here too for tests that need them?
I think we should add a helper to do this:
$this->container->get('theme_installer')->install(['stark']);
$this->container->get('theme.manager')->setActiveTheme(
$this->container->get('theme.initialization')->initTheme('stark')
);
as it's pretty tedious boilerplate.
I already had but forgot to git push.
Figured out the blocks issue.