The change in the merge request and what the title says doesn't really correspond. the body field *storage* is regular default config, what this changes and removes is the automatic addition of the field itself, aka the per-bundle auto-configuration.
Merged.
Berdir → made their first commit to this issue’s fork.
This seems quite beneficial in early testing on our project.
I have some more ideas on improvements, including caching the generator output when it's enabled for multiple menus*.
We'll test and monitor this on production, but more testing is welcome too, the impact of this should be better the deeper your path hierarchy is, for example with multiple hierarchical categories that have articles below.
* This should be avoided whenever possible to limit the overhead of this, typically only some sort of main menu needs to support trail by path, while many other menus (e.g. footer and header menus) only need none or even no active trail, which can greatly improve performance.
I updated the test, I don't think we need to remove anything, now the code that would have triggered the error only runs on the manage permissions page, so it IMHO makes sense to visit that. I had to update the assertion on the dblog page since there is no access denied log entry anymore of course.
Not sure we need more functional testing, we had a confirmation back in #14 and fundamentally, this is still the same change, it was RTBC before too, we really just removed some changes that are already done and updated the test a bit.
Reviewed. Tagging this with "Novice" seems.. brave, lots of special cases here to think about.
That is a Drupal 12 deprecation and not needed for D11 compatibility. And it breaks 10.2.
Instead, we should set up .gitlab-ci.yml with the sed trick from https://git.drupalcode.org/project/entity_usage/-/blob/8.x-2.x/.gitlab-c... so we can force tests for those modules to see how things look.
Berdir → created an issue.
Berdir → created an issue.
This has been tested and confirmed.
You should look at the referenced core issue and test and report if that's working for you.
Did quite a bit of work on this. Tests are all green now on all enabled core versions.
* Opt-in for next minor and major, previous minor and concurrent testing (speeds up phpunit job from 5min to 2min). Personally, I recommend removing next minor/major testing from CI, because it's unlikely that merge requests will break future versions (unlike previous minor (and major, once D11 is out), instead, what I do is set up a "next" weekly schedule with both of those enable, because if things break here it's because core changes.
* Workaround for mysql errors around triggers with DER, should be fixed in gitlab templates soon as well.
* workarounds for testing on next major with various test dependencies that aren't compatible yet
* Various test fixes, mostly those tests already failed at least since 10.3 too
* Removed image_upload test config, it's disabled anyway but caused weird schema errors.
* reduced previous changes around entity storage and also logging. really no need to have an exception *and* an assert() and a @var docblock. Seems fair to have an assert, but a revisionable entity type must have a revisionable storage or it would not work at all.
Note that if globstar is off (the default, probably also on CI, at least I got an error from ls **/*.info.yml when testing this), **/*.info.yml is the same as */*.info.yml:
Taking paragraphs module as an example:
$ echo *.info.yml
paragraphs.info.yml
$ echo **/*.info.yml
**/*.info.yml
$ shopt -s globstar
$ echo **/*.info.yml
modules/paragraphs_demo/paragraphs_demo.info.yml modules/paragraphs_library/paragraphs_library.info.yml modules/paragraphs_type_permissions/paragraphs_type_permissions.info.yml paragraphs.info.yml tests/modules/paragraphs_test/paragraphs_test.info.yml
$ find . -name "*.info.yml"
./tests/modules/paragraphs_test/paragraphs_test.info.yml
./paragraphs.info.yml
./modules/paragraphs_library/paragraphs_library.info.yml
./modules/paragraphs_type_permissions/paragraphs_type_permissions.info.yml
./modules/paragraphs_demo/paragraphs_demo.info.yml
find with an xargs to sed might be the more flexible option here
However, also keep in mind that composer has installed Drupal in the web folder, so that's also going to find every single core and contrib dependency .info.yml.
Also, I did steal this to make contrib dependencies compatible like this:
phpunit (next major):
before_script:
- 'sed -i "s/core_version_requirement.*/core_version_requirement: \^11/" web/modules/contrib/*/*.info.yml'
(I didn't need submodules in my case)
Rebased. To answer #37, we'll need to profile this on a real-world site with lots of contact forms and also lots of other configuration, possibly both before/after the other issue.
Doing a test on our install profile with about 1300 config objects and still on 10.2, without either patch, testing on the edit page of a contact form, a single call to getConfigEntitiesToChangeOnDependencyRemoval() is about 100ms/17% and 16MB memory. With the patch applied from the other issue, it's reduced to 50ms/10%/15MB. Memory isn't changed much because it still needs to load all config objects. That's about 35% of total memory usage.
Better, but still considerable overhead and it will get worse the more config you have and IMHO well worth the price of having a visible manage permissions local task that might be empty to save 10%+ time and 30%+ memory on every manage fields/display/forms page that shows those local tasks, without even talking about the extra overhead of admin_toolbar and so on doing, the same thing many times for many contact forms.
fjgarlin → credited Berdir → .
Categories, not package. This has nothing to do with .info.yml package, it's about the project categories on drupal.org, which I've set now.
Yes, not 100% sure about the implications, but I guess that would make sense, but that's a minor performance improvement.
This is a Drupal 12 deprecation, there is no need for the helper, we'll eventually require 10.3 and can then just update.
This is a Drupal 12 deprecation, there is no need for the helper, we'll eventually require 10.3 and can then just update.
I'm a bit confused by this change.
# Cache.
cache.jsonapi_memory:
class: Drupal\Core\Cache\MemoryCache\MemoryCacheInterface
tags:
- { name: cache.bin.memory, default_backend: cache.backend.memory.memory }
factory: ['@cache_factory', 'get']
arguments: [jsonapi_memory]
cache.jsonapi_resource_types:
class: Drupal\Core\Cache\BackendChain
calls:
- [appendBackend, ['@cache.jsonapi_memory']]
- [appendBackend, ['@cache.default']]
tags: [{ name: cache.bin.memory }]
BackendChain passes cache tag invalidations through to the inner bins. That means any cache tag invalidations are now processed twice by cache.jsonapi_memory.
I can see that \Drupal\Core\Test\RefreshVariablesTrait::refreshVariables() calls a special reset() method that is not on the interface and only some memory implementations have that, but couldn't we have "just" implemented that on BackendChain and pass it through as well?
Ah, I understand, 2.3.x requires 10.3 and this depends on 📌 Fix MemoryCache discovery and DX Needs review , had not seen that. not sure I really understand that change. Having both the chain and the inner memory bin tagged with cache.bin.memory means that those inner memory bins are actually invalidated twice. And the chain isn't a only a memory bin?
FWIW, defining this on a no_ui field type is pretty pointless, since the category is for the ui, which is disabled here, so the best option might be to just remove it, no concerns about backward compatibility then, but I guess you already require a recent version anyway.
Follow-up: 🐛 Invalid service definition for cache services Needs review
Created a merge request.
As mentioned on slack, I'm very confused that this didn't cause any test issues?
Merged.
This looks good to me. I verified by manually changing dependencies in a vocabulary config to NULL through config API, then resaving in the UI.
I'm not sure about test coverage or even an assert, because the thing is that nothing needs to be done if we just let the save proceed, it will automatically fix itself and be saved as an array. The only exception I could see is if this is in default config of a module, but not sure if we can or need to detect this. In the case where we saw this, it was a very old site with very old configuration for some vocabularies, that haven't been resaved in ages, and the taxonomy vocabulary update now resaved all them.
Not a maintainer, but if I were, I would ask that this is reduced to the changes that are actually required to enable CI and get it to pass in the default configuration. For example cspell, css changes and probably also php coding standards, at least the part that requires deprecations.
The last commit on this project was 9 month ago. Clearly the maintainer is currently not very active, and since this also blocks Drupal 11 compatibility at the moment, it should be kept as simple as possible for a maintainer to review and commit.
I also don't get the comments about 10.2 testing. That still worked in the previous MR and previous major testing also still works, what is the problem with 10.2? In my projects, I usually keep previous minor on, since I want to ensure that my projects work on supported core versions.
next major tests aren't passing, that's what I mean.
Looks like getBundleConfigDependency() doesn't like having a mix of bundle and code defined bundles. I guess that means that undefined will need to be defined as a real config entity then with protection against deleting it.
Using Gitlab UI will by default not use a commit message format that connects it back to this issue, I recommend using the UI on drupal.org, that's what I do.
Also, personally I prefer doing everything in one issue if possible, automated and manual fixes. Right now the tests are very broken, just removing the default_argument_skip_url line in default views config should fix that and then we'll see what else is left.
joseph.olstad → credited Berdir → .
Thanks, missed this, this was done in the Drupal 11 issue.
This should be done by adding config dependencies in \Drupal\entity_browser\Permissions
Duplicate of 🐛 Can't remove attached image Needs work ?
Unclear how to reproduce this. requestStack is set in \Drupal\entity_browser\Plugin\views\filter\ContextualBundle::create
Still unclear how to commit this, but all contributions must be done as MR now so that tests can run.
Make sense, merged.
Contributions must be done as MR now.
All tests are passing now. Merged.
Yes.
Contributions must be done as merge requests now, especially coding standards.
Berdir → made their first commit to this issue’s fork.
There are a lot of failing test still on D11 as many tests are integration tests with embed, IEF and others that will need to be compatible first before those tests can really be verified, but the other tests are mostly passing.
But this is in turn is also blocking others, with hard fails, so lets get this in and then we'll see.
All tests are passing except 📌 FlagFollowerUITest failing since new access policy was committed to core Active and that's failing already on HEAD and fails on all branches.
Not sure how this ended up with so many different branches and merge requests, but *shrug*.
Merged.
Merged.
I just merged 📌 Improve replicate confirmation page question to be more human readable Fixed , which at least partially addressed this and conflicts.
Not found of replacing entity with content. not everything that can be replicated is "content".
Makes sense, but needs a rebase now.
Finally merged :)
Merged.
Oops, wrong issue, reverted.
Merged.
This has coding standard problems.
A test would be great, basically add an example view to the demo module and then assert it, similar to the existing test.
Merged.
Merged.
I'm not going to add a helper function for this, this is a 10.3 deprecation for D12. We can just update this later.
Reopening this, we're running into this error as part of running updates, specifically block_content_post_update_revision_type().
Since post updates can't depend on others and if this configuration change is required for the code to not fail, I think there should either be a guard-rail in place or the update should be a regular update function and not a post update.
Will look into providing a patch when I get to it,
Switched to a merge request and fixed a wrong route name there.
See https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → and make sure you read those warnings carefully. Do *not* use the URL directly in comnposer patches.
#66 and later all makes sense to me, the MR looks good to me as well. I wrote most of that test, so I don't think I can RTBC myself, but +1 from me.
Plenty of people confirmed that it fixes their issue, all it takes now is someone confident enough to set it to RTBC ;)
This issue overlaps with 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active now, which is making the same change, with test coverage, that test coverage was deliberately written that it will need to be adjusted when this issue lands.
I think the other issue should be committed first at this point and then this rerolled.
Or, remove the findConfigEntityDependenciesAsEntities() change from this completely, then we _could_ also land this first, but it will only partially address the problem that many people are currently reporting in the other issue (it will still happen, but only when you actually visit the manage permissions page).
\Drupal\linkit\SubstitutionInterface::getUrl() does not have an options parameter, you are likely using a patch.
Patches are discouraged, update the merge request instead. Did rebase that now and added the requested conflict definition to composer.json.
FWIW, seems wrong to me to add this hack to this module. Lets focus on improving this in core.
There are are always going to scenarios that don't work with contrib. Even if the new interface would implement the old one , the opposite can still fail: module A requiring 10.3 and adding a type on UserAuthenticationInterface but module B hasn't been updated yet and overrides/decorates the service with UserAuthInterface.
Maybe there would be fewer issues but it would require another major cycle to get rid of the old interface then, we can't deprecate if if we still use it. And it would make BC even harder to detect I think, and it's clearly already complex.
IMHO, that's not in scope of this issue and there's no need to hold this up. All I'm asking for above is to add a comment to explain what we're doing and then I'm fine with either of the 3 options we have.
Yes, I don't see an issue here. Any block except the blocks that added by default by the install profile need to be placed. Since Drupal 8.0. There can be hundreds of blocks and you can place blocks as many times as you want, this is by design.
Only previous major ci is broken, merge requests are required now.
default vs full is explained in #61, CommentDefaultFormatter only adds a dependency if that config exists, otherwise it doesn't. This is inconsistent and weird, likely because of the described discrepancy between the default config and the test trait. But it's mostly a test-only thing, on a real site, you can't chose a view mode/display that doesn't exist as config, because only those are presented as options and it's not in scope of this issue.
There isn't really a point in doing this until upstream is compatible, we can't claim to be D11 compatible without that. Working with upstream has already been very painful for D10/Symfony 6, and it's clearly repeating again.
You could look into how much work it is to add Symfony 7 compatibility, you might be able to convince them otherwise with an MR that does the hopefully minimal changes and remains backwards compatible, but there's a fairly high chance that they are not interested in that.
Pretty sure this is covered by 🐛 Make exposure of translation meta fields conditional Needs work
> But I am not sure whether Drupal coding standards allow them. I think @alexpott pointed out that our BC policy allows us to rename parameters, which means we cannot rely on named parameters. On the other hand, this line is in a test, so if the parameter is renamed, the test will fail and be easy to fix.
Yes, I wasn't sure either, but it indeed felt like such a perfect use case for it, we'd need several arguments including long constants otherwise.
Lets have a quick look before going to bed, I thought. That test shouldn't be too hard to resolve, I thought.
I added all the obvious things, node type, comment type with a field through the two creation traits, a proper user with the necessary permissions, but still nothing.
After a considerable amount of debugging, I tracked it down to an inconsistency between CommentDefaultFormatter (which defaults to view mode "default"), and addDefaultCommentField(), which defaults to the view mode "full". Combined with the unusual behavior of \Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter::calculateDependencies() to only add a dependency on the view display if it exists. full didn't exist, so it couldn't load it, didn't add a dependency, then the node.article.default view display didn't depend on that and it didn't need to try and remove it from the dependency chain.
I also added a few additional asserts, but the test is fragile by design and I don't see how we could change that. There is no positive outcome to look for here, the manage permissions tab isn't accessible, nothing must happen, that's the whole point of this issue. I did write it so that it will fail in 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work , so we can extend it a bit there and check the empty message and stuff instead of a 403 on the permission page. I do hope we still agree on doing that, the performance issues are at best partially resolved with this.
#2642810: UX improvements for nested paragraphs → was the old issue for this.
We can extend/adjust \Drupal\Tests\content_workflow_bynder\Kernel\ConfigCreatorTest with a langcode that includes a -
Agreed, I think this should be adjusted in \Drupal\content_workflow_bynder\MigrationDefinitionCreator::buildMigrationDefinition instead(), and convert the language identifier there to something that's a valid id.
> In my previous comment, I suggested that, instead of deprecating EntityPermissionsRouteProviderWithCheck in Drupal core, we should implement hook_entity_type_alter() in the admin_toolbar module. I am adding an item to "Remaining tasks" to decide which approach to use.
I don't see why admin_toolbar is supposed to implement a workaround for core. The problem is in core, admin_toolbar doesn't cause it, it just exposes it more, but it's already there.
And yes, the warning from the other issue also happens when you are on any page that displays the local task and even with the access check removed, still happens on the manage permissions page itself.
> I think that replacing getConfigEntitiesToChangeOnDependencyRemoval() with findConfigEntityDependenciesAsEntities() is a good idea, but I do not see how it is in scope for this issue.
It can be argued, but the reason we remove the access check is performance and the side effects by calling getConfigEntitiesToChangeOnDependencyRemoval(). But it's not the access check that's so slow, it's the method that is called by the access check. Just deprecating the access check alone still results in a slow and expensive manage permissions page that has side effects. And inverted, findConfigEntityDependenciesAsEntities() is slightly better but it's still a slow and memory heavy operation. There's no query we can run to find this information. Drupal has to load every single config object in the system into memory and loop over them.
So, only both changes together really solve the problem (although not 100%, I'd like to not have to depend on config dependencies for this at all, but we'd need to introduce an new API for this)
The format that core uses is identical to default content, we don't need to rewrite it.
It's still internal, but we can try running our tests against the core implementation.
Reviewed.
There's a second version of this in the honeypot hook I think.
Berdir → created an issue.
Closing as duplicate of 🐛 Optimize CacheContextsManager:::convertTokensToKeys()/optimizeTokens() Active , more recent and useful I think.
Core removed any usage of the string system.drupal_js_cache_files 12 years ago when it was renamed to system.js_cache_files. And that too was deprecated and removed in in 10.2 or 10.1.
Whatever you have there is probably not core or at least not actively used anymore. Try searching your code base for it, if you don't find anything, you can delete those records. FWIW, since this is a cache collector, if nobody *uses* those state keys, they will also not end up in the cache.
deprecation message versions will need to be updated to 11.0/12.0 I guess. Leaving that for someone else while I get some sleep.
This is an interesting one as the other issue is currently critical and this is kind of the only fix for that, but it introduces deprecations and new UI text as well. The deprecation we could in theory just drop for a 10.3 commit, the empty message seems useful enough to have, although it possibly could be improved a bit.
I don't fully understand what exactly changed in 10.3, this already was an issue back in 9.4 when this stuff was originally added.
The issue that @catch wanted to link to is 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active .
I added an empty message to the page and updated to test to check for that instead of an access denied response.
I also updated the call in \Drupal\user\Form\EntityPermissionsForm::permissionsByProvider() to use findConfigEntityDependenciesAsEntities() instead of getConfigEntitiesToChangeOnDependencyRemoval(), which would likely already fix that other issue, but it's IMHO still too heavy as an access operation that's run on possibly dozens of links when using admin_toolbar.