I don't think that changes much in practice so far, if it doesn't have dependencies that might not be fullfilled, still going to be always installed?
If it's optional, we need to change node_add_body_field() to return early if the field doesn't exist and document that it might return null, which is a bit of an API change?
Maybe we could remove some calls, but it doesn't hurt to keep them, disconnect() checks if there is an open connection. If you want to make sure it works and is called, you could set a breakpoing in __destruct() (and optionally remove the explicit disconnect) when doing any operation, you should see it go in there then at some point.
Merged.
If it really is still compatible with D9 then I'm open to doing a new release with that fixed, but will need to review the changes in alpha8 to see if that's really the case.
> We're trying our best to upgrade our project, but for some reasons we can't do it now, but thanks for mentioning it.
Yeah, I can understand that this can happen with some dependencies and so on. But then I'd be very, very careful with contrib updates and only update what you have to. Because the opposite of this issue is sooner or later going to cause bigger issues for you than this, you saw this immediately. But a module that claims to be D9 compatible and actually isn't anymore is going to be annoying.
The merge plugin is discouraged and should not be used, I don't think this should be included.
IMHO decorators is an antipattern when not explicitly supported as a concept by an API/interface. It requires a base class that by default calls the inner class or it doesn't work and you can't actually have multiple decorators, so you can just as well just replace the class you want to customize. Decorating a class breaks if you add a new method and have no base class to cover that addition. Or put in other words, decorating should _not_ require to extend the default implementation, otherwise you are not actually decorating it.
If hook implementations are complex enough that it's worth reusing/extending it instead of copy & paste then it that code should probably live in a service and be an actual API on its own. Many things work like that already,
I've literally been first in line to argue against 🌱 Use final to define classes that are NOT extension points Active because I think plugins and many other things are useful to extend, but hooks are a very different scenario.
I've argued many times against against making too many things final/internal/not an API , but IMHO, it really makes sense in this case.
hook implementations are explicitly excluded from our BC policy: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → . So are constructors, but we still in updates to be as nice as possible to contrib there.
By making them final, it is very clear that we do not have to worry about BC for constructors, we can change them, re-group them (although I suspect that will break whatever alter/replace logic that we have).
Do we even know what's going to happen if you extend such a class? Isn't it still going to find the original implementation in the original module? What about other hooks that you don't want to extend? What if two different modules want to customize two hooks on the same class?
This really, really seems like a can of worms we should not open.
This still applies, however, core has been merged recently and I did that too instead of a rebase, with almost 60 commits, a rebase would be a ton of work. One option would be to squash it together into a single commit and force push that, I did that in a different issue.
You're welcome to contribute to that issue, it has clear feedback that needs to be addressed and then it can be committed.
That's a new deprecation, its actually the new syntax proposed here.
I think a new issue makes more sense
It's not possible change a release, so that can't be undone, we can do a new release, but there is no way to tell composer that alpha8 is actually Drupal 9 compatible.
I assumed drupal.org overwrites the composer.json info but maybe not if it's already there. One option would be to just remove the line in composer.json.
Let me know if this actually works for you then, or also just stick to alpha7, keep in mind that Drupal is EOL since 1 year now.
Merged.
That meta is for the non-module includes folder. IMHO this is either a duplicate of deprecating hook_hook_info() (which itself now is kind of a duplicate of deprecating legacy hooks) or it's a fairly-far-future deprecation of ModuleHandler::loadInclude()
Any added or removed paragraph is an ajax operation and it will automatically call all behaviors again on the whole widget. It's not a direct event or anything, but combined with a once() you should be able to act on paragraph changes.
Thanks for the quick turnaround. Tests would be nice, but I'm OK with merging this as it is now.
Nice, reviewed.
I'm not sure what the purpose of this issue is. We will deprecate the concept of autoloading inc files for hooks as part of 📌 Deprecate hook_hook_info() Active , but do we really need to do more?
While discouraged and not really serving a purpose, if you load them manually, it's not really an issue? I think this would essentially deprecate \Drupal\Core\Extension\ModuleHandlerInterface::loadInclude(), but for that we IMHO need to get rid of .install too, which is quite a big thing and not really that urgent?
Would be helpful to get this in, I understand that it's not strictly required, but phpstan complains and that will confuse a lot of people.
That said, I would recommend we do not just copy paste the documentation from 11.1, that is IMHO very confusing, but I'd epect that it would actually do something then on 10.x/11.0
I would clearly state that the two attribute classes only exist in those core versions that people are looking at to satisfy phpstan and as a place to document that it doesn't actually do anything beside that before 11.1.
I still believe the easiest way forward is to add a single line to the existing createDuplicate() to trigger a hook. Yes, if we'd start from scratch, this is how we might design the duplicate API, it's consistent with other processes.
But again, it is a lot of methods and BC breaks for very little gain and this issue has been stuck for years on this. I agree, a duplicate source is neat for certain use cases during save, but it's also not impossible to work around this. If you need that you can fairly easily keep track of the reference on the duplicate hook and go back to that on save. Now with the new #Hook, you could probably even simply store it as a reference on the service. And the vast majority of use cases do not actually need this.
If someone reworks this, probably easiest as a new MR by adding a single hook call, API documentation and a basic test somewhere then I will RTBC this.
This also came up in Drupal CMS, where the current clone/replicate modules were evaluated: 📌 Evaluate cloning modules Active
Merged and moved the new hooks to the new Hook class. Would be good if someone could double check that.
The crucial bit that replicate brings to the table is events that other modules can integrate with. Paragraphs for example implements that and all kind of content building that uses other entity types (also for example layout builder with block_content) requires something like that. That's for example also why commerce has its own clone features. We also further extend that in client projects to add further customization to the process, including some projects that extend the confirm form to add some extra options (for example into which group the content should be replicated.
We're also working on a generic functionality to skip translations when cloning/replicating, a frequently used feature by our editors.
The best way forward would IMHO be to get the following core issue committed: ✨ Introduce a "duplicate" entity operation Needs work , then replicate_ui could for example become just a UI and the replicate project could be deprecated.
Also the UX feedback here would be great to post into issues in the replicated modules, happy to change the redirect to the edit form instead of view for example, should be an easy change.
Maybe this could be reopened to discuss/track that?
Updated and created a merge request.
Instead of a a prefixed "internal" function, what if we already convert this to the new #Hook + #LegacyHook hook syntax? A single method can then implement both hooks on 11.1+ and the two old hooks can be marked as #LegacyHook. That's fully forward compatible and will not need to be touched anymore until D10 support is dropped.
And the total amount of tests goes from 28879 to 28526, that's about 350 less, that's quite a bit, about 1%?
Not familiar with that code, so not feeling confident enough to RTBC. It's in run-tests.sh, but this thing is fairly complex, any chance we can put this in a class and test it?
Merged.
needs a rebase.
Lets discuss that in a new issue? That said, -1 from me on that. It will make it harder to search/grep for implementations and understand for a human what on earth fieldWidgetCompleteTestFieldWidgetMultipleSingleValueFormAlter is exactly. I'd rather do the opposite and have the full Hook attribute and possibly use a shorter method name.
Merged, thanks.
that's in \Drupal\tmgmt_content\Plugin\tmgmt\Source\ContentEntitySource::getEmbeddableFields(), don't remember right now why we also cover regular entity reference paragraph fields alwaays, open to changing that, just remove 'entity_reference' there.
You could write your own translator that only supports those specific language combinations. A bit like the test implementation. You can't make an existing one like google do that for you.
Open to the idea, but I'd prefer to add something to ContinuousManager instead of undocumented properties, I have no idea when we'll manage that, but I want to deprecate and remove the ability to put arbitrary stuff on content entities.
Could be a new method to set a flag and then getContinuousJobs() would just return nothing, or an explicit method to check for that.
Merged.
Makes sense, merged, that code was written/ported before ensureDestination() existed.
Same issue as 🐛 Suggestions jobs are not created for each language in a multi-language scenario Needs review ?
berdir → made their first commit to this issue’s fork.
Missed this issue, we just fixed this in 📌 Exclude read only fields from translation Active
fetching updates is the responsibility of each service provider integration, the file translator has no fetch since it only supports upload, so you're using something else. Most integrations either support callbacks or use a cron implementation.
Merged.
See also ✨ New option "Wrap only text-like links." Active as a related/follow-up issue, which solves issues that we have where external links are on cards and similar block elements and not just in text.
It needs to land first, this builds on the other issue.
longwave → credited berdir → .
That could be a useful option, automatically including suggestions on continuous jobs. Will not be able to handle updates on those references though, so might be better to have a separate continuous job for whatever it is that you want to have translated, like medias or menu links or whatever.
> That route has the _csrf_token requirement. That gets removed by the array_diff in AccessManager::check because there is no request, which the csrf_token requirement needs.
We tried the patch and did just run into this as well.
On one hand, I think that can be improved in masquerade and is arguably even a bug. The controller still checks access, so it's not a security issue.. masquerade_target_user_access() or masquerade_switch_user_validate() really should be exposed as an access check and the route should not _only_ have the csrf access check.
But I'm also not sure that the approach here is correct. It's kind a neat to fall back to the url access, but probably also has some performance overhead, because it involves creating route matches and lookups and directly doing access checks on the entity really should be faster. And it's an implicit API change anyway because all implementations need to now always return their URL.
An alternative, more "direct" approach would be to pass around a cacheability object to getDefaultOperations (tricky to add with BC) and the hook (easy to add an extra argument) and require that implementations add their access cacheability to do that and let calls deal with that. Similar to how hook_tokens() works for example.
Per #2, automatic embedding of references should _only_ be done for specific things like paragraphs that are part of the main entity, for related things, use suggestions.
@wim: The point of this issue is that the UUID can be the ID, at the same time. That means these entities do not need to link by UUID because it's literally the same thing as linking by ID.
This got it in just before 📌 Convert entity type discovery to PHP attributes Needs review , so the new EntityTestUuidId entity type is still using annotation instead of attributes, should we open a follow-up issue or do a quick fix here?
Merged.
> With attributes, that will crash, and your module needs to implement the alter hook for its own plugins.
Which means it's not a blocker, just an inconvenience.
The thing with the third party discovery added here is is that it assumes that the plugin definition is a array-dumping-ground. object based plugin definitions need to explicitly support additional properties too, which entity types actually do, also on the attribute. They have an explicit additional array property where extra stuff can be put into, there are IIRC also a few test entity types that make use of that.
Any plugin type that wants to make it easier to support additional stuff can add that property as well.
Agree that #18 is kind of a different scope, but maybe this issue is just a more specific use case of that, and we if support additional attributes as a concept, based on a common base class/interface and have a mechanism that allows for them to control how the result is merged, then it doesn't really matter if the extra bits are official or not.
And of course, the ability to have that and implementing it for entity types would not be the same issue either, we could have a proof of concept for our test plugin types, just like this issue currently also does.
Update merge requests, not patches. Patches can't be tested and will not be committed. It's generally recommended to just store patches from merge requests locally now.
Not sure if that could be a handled in the same issue, but I think that specifically the entity type attribute could really benefit from splitting up the attribute class, to make it more similar to how Doctrine and also for example Drush command attributes:
#[CLI\Command(name: self::GET, aliases: ['cget','config-get'])]
#[CLI\Argument(name: 'config_name', description: 'The config object name, for example <info>system.site</info>.')]
#[CLI\Argument(name: 'key', description: 'The config key, for example <info>page.front</info>. Optional.')]
#[CLI\Option(name: 'source', description: 'The config storage source to read.')]
#[CLI\Option(name: 'include-overridden', description: 'Apply module and settings.php overrides to values.')]
...
public function get($config_name, $key = '', $options = ['format' => 'yaml', 'source' => 'active', 'include-overridden' => false])
Right now, it looks like this:
#[ContentEntityType(
id: 'node',
label: new TranslatableMarkup('Content'),
label_collection: new TranslatableMarkup('Content'),
label_singular: new TranslatableMarkup('content item'),
label_plural: new TranslatableMarkup('content items'),
entity_keys: [
'id' => 'nid',
'revision' => 'vid',
'bundle' => 'type',
'label' => 'title',
'langcode' => 'langcode',
'uuid' => 'uuid',
'status' => 'status',
'published' => 'status',
'uid' => 'uid',
'owner' => 'uid',
],
handlers: [
'storage' => NodeStorage::class,
'storage_schema' => NodeStorageSchema::class,
'view_builder' => NodeViewBuilder::class,
'access' => NodeAccessControlHandler::class,
'views_data' => NodeViewsData::class,
'form' => [
'default' => NodeForm::class,
'delete' => NodeDeleteForm::class,
'edit' => NodeForm::class,
'delete-multiple-confirm' => DeleteMultiple::class,
],
'route_provider' => [
'html' => NodeRouteProvider::class,
],
'list_builder' => NodeListBuilder::class,
'translation' => NodeTranslationHandler::class,
],
links: [
'canonical' => '/node/{node}',
'delete-form' => '/node/{node}/delete',
'delete-multiple-form' => '/admin/content/node/delete',
'edit-form' => '/node/{node}/edit',
'version-history' => '/node/{node}/revisions',
'revision' => '/node/{node}/revisions/{node_revision}/view',
'create' => '/node',
],
// continues on and on
We'd split up specifically repeatable but also optional and also but not only third party keys into separate attributes. Haven't really thought about how to merge it together again, since the result still needs to be a single definition.
#[ContentEntityType(
id: 'node',
label: new TranslatableMarkup('Content'),
...
)]
#[Handler('storage', NodeStorage::class)]
// Or maybe even more specific subclasses for common handlers
#[StorageHandler(NodeStorage::class)]
#[Link('canonical', '/node/{node}']
?>
I think that would be a lot closer to how other PHP frameworks deal with this and while it probably be just as long, it would be less complex and easier to format. Pretty sure that our entity type attribute class is going to give any developer familar with Doctrine a heart attack ;)
All action plugins work like that, I don't know why on top of my head.
phpstan doesn't complain about the method, it complains about a call to it. Add the correct type and it should not complain anymore.
Ah, and as maintainer of TMGMT, definitely +1 on not including it for now. Happy if people use and like it, but as mentioned, it's targeted towards more complex sites and needs integration with a translation service providers to be really useful.
Agreed on interface translation (even non-EN single language sites need that and it's automatically installed when you install with non-EN), but it's basically just enabling the module. Same for config translation, that doesn't really need configuration, each config entity defines what's translatable, everything else is generic. (Minus the complexities and bugs and problems when working with config then, but recipes can't solve that)
I'm not sure why adding a specific language would need to be a recipe, configurable or not. The recipe is the ability to have multiple languages and necessary configuration for that. The specific languages can then be added through the regular UI, that seems like a non-issue to me. It's a single step, the recipe could display a message to guide the user to the next step or so? This might be a general thing that I haven't really seen yet, that we tell the user what the next step is after a certain recipe.
URL language negotiation mostly configures itself once enabled, the prefix automatically defaults to the language code, the only thing I'd recommend is that the default language is also set to a prefix, because by default it doesn't have that. And that doesn't play well with browser language negotiation and caching.
The main complexity here is IMHO content_translation configuration, that's where the bulk of complexity is. Because it basically adds an extra dimension to each content type/bundle that's set up. The issue summary talks about entity type and field type settings, but that's not how translatable config is stored, it's stored per bundle and per field. Each bundle needs:
* the opt-in to enable content translation (new config entity with a few settings like default langcode and show/hide untranslatable fields, which also relates to content_moderation, which enforces that). Not every entity type/bundle needs to be translatable, depending on your use case and scenario (e.g. comments and users can be translatable, but most projects won't need or want that, you might have node types that aren't translatable)
* each field needs to define whether or not it's translatable. this is base config, while not exposed in the UI, each recipe can be configured to set sensible defaults (e.g. body translatable and a tags field isn't). But this setting also exists for base field like changed date and title, those are then stored as field config overrides.
* Some field types like image support a mix, so the field as a whole is translatable, but the file reference is shared/synchronized and the title/alt text is translatable/different. That's a third party setting on the field config entity, depending on content_translation, so can't be prepared.
* Depending on the use case, there are different ways to filter views. Either you only show translated content, or you show all content once, with a translation if it exists. That means different filters on views that by default like won't have them. As a default, all views should probably default to showing the default translation only, that shouldn't affect single-language sites and then sites can customize from there.
* Translation permissions, although this can also be done fairly well with a global permission at this point (translate editable entities)
Constraints:
* Multilingual content translation recipe can't depend on all other content type recipes.
* This set up should IMHO work independently of recipe install order. We can't expect that sites will first enable all the content type recipes and then go multilingual. They might later on add an event recipe and that should then also be multilingual.
I can vaguely think of 3 general approaches to that:
1. Generic code/magic/assumptions. That sounds more like a new (core?) feature than a recipe. Something like a checkbox somewhere that says that all node node types (setting per entity type? or maybe inverted, something like a checkbox to customize the configuration) and their fields should make sensible default assumptions about translatability based on field type. But I expect there will be things that are off, and if it's like the mentioned customize page, once it's off, it's off.
2. An additional foo_translatable recipe for each foo recipe that depends on foo and multilingual. That will need a lot of additional recipes and IMHO would require some kind of suggestion system like "You just applied the content translation recipe, you might also want to apply Article Translation, Page translation, Tag translation, ..." and also in reverse, "You just applied the event recipe, (since content translation is enabled) you also might want to apply Event translation.
3. Optional/conditional config and config actions in a recipe, similar to optional config in a module that gets applied once all dependencies are met.
Both 2 and 3 would require a system that keeps track of applied recipes to make suggestions/apply their optional parts once the requirements are met. For 3, that would means enabling content translation would revisit a lot of previously applied recipes and set up the necessary extra config entities and third party entries. Option 1 might sound like the easiest, but it's a new thing that AFAIK doesn't exist yet and I would expect that there will always be exceptions where the generic assumptions won't work.
For our project we always keep English as the default language, as a matter of fact we prevent changing the default language to something other than English. This has saved us a lot of time, because it prevents configuration chaos, when you introduce configuration translation.
I can see how that does make sense as an agency, but that's not really something that can be enforced IMHO. Sometimes you start with a single-language project that's not English and then you need another language. The installer asks for the language when you install, I don't think we can or want to disallow that. That process requires that even for single-language sites, you need to create everything EN first and then translate it. Drupal also needs to be available for users that don't speak English or at least not good enough.
+1, saving config entities through config API will bypass API's and things might not work as expected. For example, I tried to add workflow configuration but caches weren't invalidated because content_moderation_workflow_update() didn't run (I did then find the specific config action to do what I wanted).
One thing keep in mind is that set() does not offer the same functionality, specifically it can only replace a whole top-level property, while SimpleConfigUpdate supports the usual dot based syntax to set child keys directly. Maybe set() should be expanded to have similar capabilities.
The client confirmed it worked for their use case, but I didn't test it that extensively, and specifically didn't test if it still works for any other use case.
Feel free to leave it at needs review for others to confirm or just only fix it for 4.x, your call.
Confirmed that it's apparently only the "foo is not a directory" message that's not an error.
berdir → created an issue.
Not entirely sold on this. Aren't easy and simple words that should be avoided in documentation?
The first sentence seems to be just as much about what it isn't and doesn't seem to explain too much IMHO. What exactly are "basic tools"? And I think starting with the components and site builders before editors makes more sense.
What about
Allows site builders to create components, such as cards, images and more. Content editors can compose flexible but structured content pages from these building blocks.
This is missing the replicateEntity() call now.
Merged.
Removed, thanks.
That's not supposed to be a merge requests, that's just a workaround to achieve that goal.
Missed that 3.x is now compatible, that makes more sense than my workaround. Rebased to remove my skipping.
Contributions need to be merge requests now.
I don't see what this has to do with published, note that you could also change a paragraph from published to unpublished and unpublished paragraphs are shown to editors.
This will add a lot of extra cache tag invalidations, but cache invalidation should handle that and avoid unecessary calls.
berdir → made their first commit to this issue’s fork.
Merged, disabled IEF tests on D11 for now.
Forgot about your comment, we did actually run in the paragraphs issue again and created 🐛 When a translated node is duplicated with no translation, the paragraphs are still translated Active , I don't think a separate event is necessary.
Does the item type support a description? If so then put something like "Use this when setting up the Bynder OAuth App for the Authorization redirect URIs field". If not, add something short in the title, like "(Authorization redirect URI)"
Merged.
That was maybe a bit unclear, let me expand: "I assume that API module currently out of the box would not find class methods as hook implementations, whether or not there is a 'Implements hook_foo()' docblock and therefore having or not having those docblocks is not relevant or api.module. It needs adjustments to find those and these adjustments will not look at the comment string anymore.".
Opened 🐛 Wishlist link adds #attached to lazy builder, throws AssertionError Active but realized the issue is introduced by this issue.
Pushed a quickfix but I think the cache contexts aren't necessary and should be moved into the lazy builder, as it is now they might bubble up and cover the whole page. But I'll need to do more testing on that.
@agoradesign:
> I maybe the wrong person to answer this, as I keep struggling for years to find a way to find the right cache metadata for cart links in the page header, that show the number of items in the cart. Everytime I thought, I solved that, it happened again, that the counter kept staying at 0.
What we do in custom block in a project is the combination of cart cache context + cache tags from the cart(s):
/**
* {@inheritdoc}
*/
public function getCacheContexts() {
return Cache::mergeContexts(parent::getCacheContexts(), ['cart']);
}
/**
* {@inheritdoc}
*/
public function getCacheTags() {
$cache_tags = parent::getCacheTags();
$cart_cache_tags = [];
/** @var \Drupal\commerce_order\Entity\OrderInterface[] $carts */
$carts = $this->cartProvider->getCarts();
foreach ($carts as $cart) {
// Add tags for all carts regardless items or cart flag.
$cart_cache_tags = Cache::mergeTags($cart_cache_tags, $cart->getCacheTags());
}
return Cache::mergeTags($cache_tags, $cart_cache_tags);
}
So you vary by cart and invalidate if the cart changes. This has been working well for us.
Ah, this is actually the result of a patch that's being used, I'm moving this there.
Beside removing a lot of complexity (when and when not to reset cache) and lines of code, it also uncovers bugs and incorrect assumptions in tests when we forget to reset the cache.
For example https://git.drupalcode.org/project/drupal/-/merge_requests/10075/diffs?c.... That test never actually tested what I thought it did, because it was always testing against the original stale entity.
To summarize the slack discussion, this actually happened in in a kernel test that was specifically testing installing a module. Installing a module is always a special case and all bets are off.
This isn't about config entities, it's not about static cache. It's about the fact that installing a module builds a new container and all your existing services and things stored in services are now stale.
@joachim confirmed in Slack that using the static ::load() works without any resetting in that scenario, so would \Drupal::entityTypeManager()->getStorage()->load() as that's just a wrapper around that.
The purpose of this trait is mostly to help with web tests and saving entities in a different process so we have no mechanism to be notified about that. I don't think it needs to cover this special scenario. In fact, I'd rather do #3040878: Reset static entity cache on POST requests in tests → , which should remove the need to manually reset the cache in tests in 95% of cases (entirely made up estimate) and then we can just straight up deprecate this trait.
contrib doesn't have 0.x major versions, it starts with 1.x AFAIK, so numbers starting with 0 wouldn't exist. They also wouldn't work, the update number is a single integer, and right now the lowest number that you can pick is 8001 (for now, it references an issue to remove that requirement). There is AFAIK an issue to change that, but anything else is entirely up to you.
I do agree that the documentation can be improved, the part about core compatibility can be dropped, it's only about your own module.
I think the most sensible thing is then to just start with 10000 or 1 major 00 minor and 00 for the update number. I'd not put in patch for contrib modules because that will just require you to update it whenever there's a new patch version.
Some of these were addressed in 🐛 Wordbee file name collision Needs review now, I think the url() calls not yet, but the change is not correct, it needs to be toUrl()->toString() now, so should be rebased and updated.
Merged.