> PHPStan doesn't like it when there is only one file and it *.api.php.
Urgh! is there an issue for that problem?
> in the current MR as well as in Core in a few places
Are there any existing filters in core which work OK with screen reader announcements? If so, the new script should take from those.
Looks good, not sure why the bot is unhappy as the MR pipeline has passed.
joachim → created an issue.
Oh. There are two get() calls and they're not the same. That's why I'm confused!
$date_occurrence_field = $entity->get($field_name)->get(0);
$entity->get(FIELDNAME) gets us the FieldItemListInterface for the field. That one's fine, right?
FieldItemListInterface->get(0) is one of those weird magic ones maybe?
Maybe we should be calling isEmpty() on the FieldItemListInterface instead? Can you try that?
Sorry, I misread the error message.
> Call to a member function isEmpty() on null
That means that the $entity->get(FIELD) failed.
Why is the field missing? Can you debug?
It sounds like the computed field is not correctly reporting that it's empty?
LGTM!
The @link syntax is for linking a topic. I'm not sure it'll work with a method name.
Just giving the full class name and method will make a link on api.d.org.
🌱 [META] Convert some tests into Kernel or Unit tests Active is maybe the meta-issue you were thinking of?
Good start.
But we should keep the mention of hook_menu_local_tasks_alter().
Something like:
> Additionally, hook_menu_local_tasks_alter() can be used to alter local tasks dynamically before they are rendered.
Just catching an error and returning nothing is not the right fix.
What is causing the error? What should we be doing differently?
Ok it looks like it IS Views that's somehow the problem, as if I add this kernel test on this branch, it passes:
public function testRouteBuilderRebuild(): void {
$this->container->get('router.builder')->rebuild();
$this->assertTrue(TRUE);
}
Performance is unchanged, as it's still a single assert().
The potential performance change was my initial suggestion to put a foreach() loop with the assert() inside.
joachim → created an issue.
I've rebased the branch on 3.1.x, rather than have a merge commit. In general, rebasing produces a cleaner history for a feature branch.
Though now I revisit the code, I'm not sure about this bit:
'#cache' => [
'contexts' => $view_cache_metadata->getCacheContexts(),
'max-age' => $view_cache_metadata->getCacheMaxAge(),
'tags' => $view_cache_metadata->getCacheTags(),
],
Surely that's polluting the outer render array -- it should be inside instead?
Wacky idea: what if we WROTE PHP code for field access on cache rebuild, the same way Twig files are compiled?
We could then write PHP setters and getters for all the fields an entity type has.
> @joachim, please take a look if you want to adapt render elements to current backend.
I'm not sure when I'll have time.
I'm feeling a bit discouraged about this also, as it feels like doing the work I did all over again.
Can you tell me which properties of ListFilter are for functionality your JS supports, and which should be removed?
- #library for example can be removed, as you said in Slack that your JS handles sibling groups and details.
What about #minimum_filter_length and #search_start_of_words? I can't tell from reading drupal-filter.js whether it handles those -- the code could really do with more comments.
I've decided not to wait for figuring out how to deprecate plugins.
Instead, we can use alter hooks to ensure that plugin IDs are maintained for node and node_type links. We can remove those in due course. In fact, there is already this exact same handling in node module currently!
A separate problem is that core/modules/views/tests/src/Kernel/TestViewsTest.php is failing on a recursive router rebuild, because of Views. The chain goes like this:
- views data needs to know about routes
- the router is built
- MenuRouterRebuildSubscriber tells MenuLinkManager to rebuild() links
- that calls our new EntityMenuLinkDeriver
- BaseEntityLinksProvider checks routes for things
I'm not sure why this circularity is only coming up in Views, and only in that kernel test. After all, I don't get an error when I go `drush cr` with this MR.
I'd have hoped we'd have the `main` branch sorted by the time 12 comes around!!!!
But yeah, that gets a bit more complicated.
We can't say `preg_match('/\d+\.x/')` because that would catch the old versions 5.x etc.
But `preg_match('/\d{2,}\.x/')` should be ok.
So we probably want to say that `preg_match('/\d{2,}\.x/')` is greater than anything.
When 12.x comes around there won't be an 11.x around any more, so we don't need to think about that.
AFAICT you can fall back version_compare() unless BOTH left & right are 11.x and 11.0.x (though not sure what order they come in, might have to check both orders).
so:
// By default, version_compare() returns -1 if the first version is lower than the second, 0 if they are equal, and 1 if the second is lower.
if (left == 11.x && right == 11.0.x) {
// We want 11.x to be greater.
return 1;
}
if (left == 11.0x && right == 11.x) {
return -1;
}
return version_compare(left, right);
Looks good. Thanks!
> For collections of content entities, in general those probably fit best under Content; If it's a collection of config entities, those could live under Structure if they facilitate structured content (aka bundles and fields), but those could also live under Configuration if they are not related to structured content.
Are you suggesting we split DefaultConfigEntityLinksProvider into two classes,
- one for config entities for structured content, under Admin > Structure
- one for config entities that live under Admin > Configuration
In which case, for Configuration config entities, we'd need an entity property to specify the base route.
> by default, Gin only left-aligns dropbuttons within a .node-form wrapper
Why?
Buttons in forms will typically be left-aligned. There's nothing special about node-forms.
It works for selecting a paragraph type, but not on the Module builder form:
LGTM, thanks everyone!
Docs will need changing.
For example, on EntityTypeInterface::getKeys()
* - id: The name of the property that contains the primary ID of the
* entity. Every entity object passed to the Field API must have this
* property and its value must be numeric.
Not sure about the test failures -- web/core/modules/system/tests/src/Functional/Menu/LocalTasksTest.php is failing for me locally on 11.x
joachim → changed the visibility of the branch 3028968-create-js-library-filtering-lists to hidden.
Thanks for the suggestions!
I've added functionality for allowing 3rd-party (let's go with that!) attributes to override a main attribute property, if that property is marked as deprecated.
This is the mechanism that will allow us to move properties like field_ui_base_route from the main attribute into a 3rd-party attribute.
@finnsky your MR seems to have dropped the ListFilter.php render element I'd added. I feel that having a dedicated render element gives clearer DX than just adding attributes. Also, it means that you get to choose where to position the search element.
@rfay, indeed, @joachim will point out that https://github.com/joachim-n/drupal-core-development-project does it without making changes to core's composer.json. I've filed PRs on justafish/ddev-drupal-core-dev to make it work with that Composer project template.
@solideogloria you can install Drush into the project yourself.
In some cases, the invalid plugin is expected -- such as the case handled by 🐛 Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency Needs review . I thought using exceptions as messages was a bad pattern?
Create a node, then look at content admin, then click a dropdown.
The things behind the dropdown show through it.
Go to content admin.
The dropbuttons don't have an outline or a background.
Thanks for the hint! Got a unit test working.
joachim → changed the visibility of the branch 3387322-allow-contrib-extensions-on-11x-detect-dev-version to hidden.
joachim → changed the visibility of the branch 3387322-allow-contrib-extensions-on-11x to hidden.
I can confirm #17.
I ran the test src/Functional/Update/Group2to3UpdateTest.php --filter=testEntityReferenceFields (just because it seemed the simplest and quickest), with the patch from ✨ add a way to manually use the test site from a Functional test Needs review applied to core, so that the DB is preserved after the test.
I found these rows in the {key_value} table of the test DB after the test:
config.entity.key_store.group_content_type uuid:000b9028-9efe-4a4f-9552-4ed2343481d5 a:1:{i:0;s:40:"group.content_type.class-group_node-page";}
config.entity.key_store.group_content_type uuid:1891ac47-eae0-4075-b85d-38c7ca8b5122 a:1:{i:0;s:41:"group.content_type.class-group_membership";}
config.entity.key_store.group_relationship_type uuid:000b9028-9efe-4a4f-9552-4ed2343481d5 a:1:{i:0;s:45:"group.relationship_type.class-group_node-page";}
config.entity.key_store.group_relationship_type uuid:1891ac47-eae0-4075-b85d-38c7ca8b5122 a:1:{i:0;s:46:"group.relationship_type.class-group_membership";}
These values are used by Drupal\Core\Config\Entity\Query\QueryFactory::getConfigKeyStore() for fast lookups of config entities.
Added control of this feature with a BROWSERTEST_PRESERVE_SITE env var, and added documentation to phpunit.xml.dist.
Resolved all points from @xjm in a new branch 3224276-11-move-entity-helpers-to-KernelTestBase off 11.x.
I'm coming round to the idea of a trait -- it means it can be used in functional tests too.
Making a new branch for that approach.
Also, once this is in, we can add #3267404: Add a PHPUnit assertion that an entity is valid → to this new trait too.
joachim → changed the visibility of the branch 3224276-10.1-move-entity-helpers-to-KernelTestBase to hidden.
joachim → changed the visibility of the branch 3224276-move-entity-helpers-to-KernelTestBase to hidden.
Key module is just for storing API keys.
You declare a key as a config entity. It can then be obtained in a variety of ways -- set in config for local development, or from a file or environment variable on prod.
Converted this to an MR with the patches so far committed.
I can see a corner case which this does not handle yet: suppose two locked items have the *same* weight.
It should not be possible to drag an item in between these two items, because there is no way to set a weight to go in between them.
There's a core patch for this at ✨ Support disabled/locked items in tabledrag Needs work !
I really like this change, but I'm not sure about it being yet another setting. Our admin UI can be opinionated!
And I don't understand the bit about before/after local tasks.
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/BooleanCheckboxWidget.php
@@ -38,6 +39,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+ // Strictly speaking the checkbox widget should only be available for
+ // required field definitions in the first place, but enforcing this would
Why?
I don't understand this at all.
If the checkbox is 'Agree to site terms and conditions' then yes.
But a checkbox could be 'Promote to front page' or 'I want a child meal'.
Looks like, yes!
joachim → created an issue.
Ah right, you mean so that a kernel test in a contrib module will accept to enable that module in the test site?
I hadn't actually thought of that! I'd like to get this in and leave that to a follow-up!
> ddev drush cset -y package_manager.settings include_unknown_files_in_project_root true
I presume that copies over any files from the project root over into the staging project? If there is a clone of Drupal core in repos/drupal, then that's a LOT of files to copy!
I think amending the path repo to point to the same place would be simpler and quicker!
> There are probably some comments about how to correctly use Drupal API that cannot have another place to be posted
The best way is to file a docs issue and improve the docs so they are clearer.
> Would it not be simpler to use the same language and style that the ProxyBuilder uses for building files?
Could you be more specific?
I've previously looked a bit at ProxyBuilder, and the whole area of proxy services is one I find to be pretty badly documented. I don't really understand why ProxyBuilder exists or how it does what it does. Also, ProxyBuilder has to write much more complex classes than in this case -- here we just need to replace tokens with strings. ProxyBuilder has to write methods and all sorts.
> I think the class LocationsClassTemplate could be in the manager file.
I wanted to keep it separate for clarity. That way the class that holds the code for the DrupalLocations file is just about that code.
> I am not sure that 'manager' is an appropriate name. I'll have to look again at another time.
I followed the same pattern as ManageGitIgnore, also in the scaffold plugin. ManageGitIgnore is responsible for creating a .gitignore file. So ManageDrupalLocations is responsible for creating a DrupalLocations file.
> I don't see history here for why there is a change record 'Obsolete ...'. What ever the reason, in the future just update the draft CR on an issue. Having unwanted extras adds to the difficulty in managing the change records.
The approach was completely changed. I don't think it would make sense for the node history of the CR node to go from saying one thing to saying something utterly different. The obsolete CR can be deleted.
I'm not sure about your suggested change from 'app root' to 'application root'. I made the changes and then thought to check core -- in 11.x there are LOTS of uses of the term 'app root'. So I'm wondering about whether to revert those commits I just made, and keep consistent with what's already in core.
Also, your suggestion for this doesn't work:
* The web root of the project. This is either defined in the project root
* composer.json, or taken to be the project root by default.
It needs to be a single sentence, I think.
> You download the module and its additional necessary libraries (defined in the module's composer.json file) with Composer.
I very much support efforts to define and clarify terminology, but it's going to be hard in this case because Composer itself talks of 'Installing dependencies' and has the command `composer install`.
So I think we need to disambiguate the term 'install' in Drupal.
@quietone I'd specifically put 'enabled' because 'installed' is ambiguous - people were thinking this issue was about installing with Composer, when it's not.
Rebased.
If I'm right about the problem, then the fix would be for the process that copies the project's composer.json file to check the 'repositories' property for any repos which are path repos, check those for any relative paths, and the prefix the absolute directory path of the project to that URL in the temporary copy.
If I understand it correctly, the problem is that Project Browser creates a temporary Composer project in a temporary folder, using a copy of the project's composer.json file.
In joachim-n/drupal-core-development-project, Drupal core is coming from a Composer path repository, which is declared in composer.json with a relative path.
So I think this problem will happen if there are *any* Composer path repositories -- e.g. with https://github.com/joachim-n/drupal-project-contrib-development, or if there are any other packages being used like that.
I don't think 🐛 Wrong DRUPAL_ROOT with non-standard code structure RTBC is relevant here, as that is to do with fixing Drupal core so it works properly when using a path repository.
> Btw, to see how a lack of documentation looks like, we only need to look at the symfony codebase.
> I find this painful to read :)
Yup. Symfony code is horrible to work with.
> What information do these 12 lines provide which
The word 'within'.
This was a bug in Computed FIeld -- fixed in bda64d32cc09b88e00f3cd508d5cb91d5cf56ef.
> The reason that we don't want to require drupal/core with self.version is that it makes upgrading harder.
In what way?
We now recommend that upgrading core be done with "composer update drupal/core*" which updates both packages simultaneously. Won't the dependency work ok with that?
Looks good!
It looks like the reason the core ConditionManager service is overridden is to add additional annotation namespaces to the annotation reader:
// Make sure to add our namespace first, so our ContextDefinition and
// Condition annotations gets picked.
$this->annotationReader->addNamespace('Drupal\rules\Context\Annotation');
$this->annotationReader->addNamespace('Drupal\rules\Core\Annotation');
I'm not sure why these need to specifically go *first*. If they didn't, then the $additional_annotation_namespaces parameter could be used in the plugin manager's __construct(), and that **DOES** have BC handling.
Also, why does Rules need to put its annotation classes into silly places?
The problem is not that Rules overrides the getDiscovery() method. Lots of core plugin managers do that too.
The problem is that Rules swaps the ConditionManager service for its own class:
public function alter(ContainerBuilder $container) {
// Overrides the core condition plugin manager service with our own.
$definition = $container->getDefinition('plugin.manager.condition');
$definition->setClass(ConditionManager::class);
}
and that class changes the discovery:
// Swap out the annotated class discovery used, so we can control the
// annotation classes picked.
$discovery = new AnnotatedClassDiscovery($this->subdir, $this->namespaces, $this->pluginDefinitionAnnotationName);
$this->discovery = new ContainerDerivativeDiscoveryDecorator($discovery);
which means that Condition plugins in core that are since 10.3 defined using attributes aren't getting picked up, since the replacement ConditionManager service isn't using attributes.
I'm still iffy about the EntityQueryRelationshipMultiplePropertyBaseFieldTest test class being empty, but I don't think there's a better way to do it.
I tried a PHPUnit data provider with the code from createField() in a callable... but PHPUnit didn't like that at all!!! :D
Rebased.
Deprecations still to do.
The preg_replace() callbacks such as _filter_url_parse_full_links() should be deprecated too.
Yup, the MR at 🐛 Entity field relationship queries for multi column field items stored in shared tables are broken Needs work refactors addTable() to have a simple loop that consumes the pieces of the field specifier. Once that gets in, it'll be MUCH easier to add an additional check to that loop to check for the 'referenced_by:' token.
Made a number of tweaks -- mostly docs & formatting fixes.
Uncommented the deprecation.
Also, addressed this
> I'm a bit spooked by this class property which gets overwritten each time a new field is added to the query.
by removing the $this->joinType and using a passed parameter instead.