#Needs tests

The issue still persist with the Drupal version 10.4.1 and with the latest version of Flag i.e. flag 8.x-4.0-beta6 the patch added to #12 is not applicable.

Hi !,

Have you found a simple solution for Drupal 10.2 / 10.3.... ?

Thanks for your replies.

Automatically closed - issue fixed for 2 weeks with no activity.

Haha, I’m with you on being efficient and avoiding unnecessary duplication of resources.

We could refactor EntityCacheTagsTestBase so that the cache bins are stored in a class member variable within the setUp() method, which is a common practice. However, I always try to be cautious about not straying too far from the original scope.

You have a lot of experience, so I’m always a bit worried about overlooking something.

Let’s see if others can share their thoughts, and we’ll decide from there. :)

Thanks so much for all the effort you put into reviewing so many (often complex) issue queues!

Think my one comment still stands but will leave in review for others. Certainly not a hill I’ll die on lol

I was hoping to store this in a base field definition override rather than a separate config object - to sidestep needing to add a schema and an update hook.

What was the reasoning for that not being suitable?

If you look at our docs for menu links → - that override should be possible via a base field definition override - https://www.previousnext.com.au/blog/overriding-base-field-labels-and-de...

Drupal 7 is end of life → and no longer supported.
Closing the issue as outdated.

Drupal 7 is end of life → and no longer supported.
Closing the issue as outdated.

Just uploading a patch for the release 4.8, leaving as NW since the tests are required.

Re #6 - yes absolutely. Core has settled on that pattern with the generic revision UI.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

This looks fine and I think we can live without the drupal_static_reset() bc layer given there are no known usages at all.

Committed/pushed to 11.x, thanks!

catch → closed merge request !10910

So, are you suggesting that the "View own account details" permission should be granted to authenticated users by default? That could make sense, but it would also mean that users would suddenly see their roles appearing in their own user edit forms.

If that’s the intended behavior, an upgrade path should be implemented. However, I’m not entirely sure if that would be the best approach. What do you think?

Added some additional comments after doing some code standard reviews. I didn't resolve any of the open threads but if we can do that next please.

You're adding a new permission that no one will have

Thank you for following up @victonator. Going to close out for now. If anyone is still seeing this issue in D11 please reopen.

@smustgrave I don’t see how users could suddenly lose their access. The only users who had view+edit access to roles in user edit forms were those with the "Administer permissions" permission, and that remains unchanged. As far as I understand, the only addition in this change is a read-only view of one's own roles for users with "View own account details" permission.

Regarding REST APIs, we’ve only added an AccessResult::allowed() when the user has "View own account details", while keeping AccessResult::neutral() otherwise, which aligns with the previous behavior.

Could you clarify where exactly you see a potential permission loss?

Thanks!

I rebased again due to a merge conflict caused by this morning's commits to Core. This was an easier rebase than the last one, so I'm leaving it at RTBC.

Believe this will need an upgrade path + update test. Currently with this change think you currently have access to you would have it ripped away. Believe proper approach would be that things shouldn't change for existing users but can be configured to take away after the fact by the site admin.

Trying to combine this MR with the one from ✨ Support revisions and content moderation with the Layout Paragraphs Builder Field Formatter Needs work as both deal with the same part of the widget that needs some love. I noticed a discrepancy in how one method has a fail-safe for missing translations and the other does not. Will fix in the combined patch that I'm working on to see if it holds up.

catch → closed merge request !10619

Committed/pushed to 11.x, thanks!

We put the class name in the plugin definition, it's not too dissimilar from that - something that is needed in order to construct the plugin? To me the plugin definition is any metadata that's required before the plugin needs to be constructed, and the autowire flag fits into that category.

It would be nice to reuse the existing #[Autowire] attribute from Symfony on the class, but that's only allowed on parameters:

#[\Attribute(\Attribute::TARGET_PARAMETER)]
class Autowire

If we do go this route this would mean having a new name - #[AutowiredPlugin]?

We can flip the default either way, either globally or on a per-definition basis I think. But we haven't done that for services, I don't know if we would do it here or not.

Looking for reviews/opinions on whether adding autowire as a boolean property to every plugin definition is the correct solution here.

1. Could we put it in its own attribute per #51?

2. Could we flip the default in a future major release? Seems worth a spin-off issue even if we eventually decide not to do that.

@larowlan could you maybe take a short look, so we can finish this? (Whenever you have the time) Thanks! :)

> 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.

anybody → closed merge request !29

Let's merge the tests nevertheless! :)

Reason seems to be in frontend theme, something like this:
🐛 Address output missing spaces (e.g. first / last name) Active

The problem was related to an internal theme bug. Not related to glossify.

Neither through the tests nor through manual testing, I can reproduce this issue.

Thanks, anyone who needs this please provide a MR with tests. Thanks! :)

Thanks, anyone who needs this please provide a MR with tests. Thanks! :)

I'll take a look.

The previous MR was very out of date with a lot of merge conflicts and merge commits. I've squashed everything on to a new MR and hidden all the old branches. I think this is still NW but now we're up to date :D

acbramley → changed the visibility of the branch 2473873-views-entity-operations-10.1.x to hidden.

acbramley → closed merge request !3556

acbramley → changed the visibility of the branch 2473873-views-entity-operations to hidden.

acbramley → changed the visibility of the branch 11.x to hidden.

acbramley → changed the visibility of the branch 2473873-views-entity-operations-10.1.x to active.

acbramley → changed the visibility of the branch 11.x to active.

acbramley → changed the visibility of the branch 2473873-views-entity-operations-10.1.x to hidden.

acbramley → changed the visibility of the branch 2473873-views-entity-operations-10.3.x to hidden.

acbramley → changed the visibility of the branch 11.x to hidden.

Coming back to this issue after a long time, it seems like 🐛 Array to string conversion for Media Image Needs work is the correct fix. With that patch I can re-edit an embedded media entity and it correctly removes the empty string settings variable.

Looking for reviews/opinions on whether adding autowire as a boolean property to every plugin definition is the correct solution here. This lets plugins opt in individually which solves #47, and I now think may be required as per #48.2, because we can't guarantee to infer correctly in all cases from interfaces alone.

If we do do this I also think we have to keep the create() methods around for now because there might be subclasses calling parent::create(). There is an attempt in the MR at a BC layer for this, because I think we can and should start removing the interface when the plugins are autowired, but the interface might not be explicitly added to subclasses.

You're right, and now I've seen that a isset() guard was added in 🐛 There is no way to delete file entities of other users Fixed after this issue was created, so this is not longer a problem. I think we can close this issue as outdated.

  if ($account->id() == $file_uid[0]['target_id']) {

Was changed to:

  if (isset($file_uid[0]['target_id']) && $account->id() == $file_uid[0]['target_id']) {

It was better to check for $entity->getOwnerId() rather than $file_uid = $entity->get('uid')->getValue() and $file_uid[0]['target_id'], but I think that's now out of the scope of this issue.

Then I would say not. If the test coverage were adequate it would catch the bug reported here. So think the coverage needs to be expanded to cover the scenario described.

Thanks for pointing it out. Now I see that \Drupal\Tests\file\Kernel\AccessTest already tests the update operation over a file with and without owner, so I believe we have this covered! :) haven't we?

I've debugged step by step and checked that the new code is exercised.

    ...

    $this->assertFalse($file1->access('update', $user_own));
    $this->assertTrue($file2->access('update', $user_own));

    ...

    // Create a file with no user entity. 

    // <<Creates $file4 with no uid>>

    $this->assertFalse($file4->access('update', $user_own));
    ...
    $this->assertFalse($file4->access('update', $user_any));

Trying to get feedback.

Sorry, I completely overlooked the second point in #18.

I've implemented it, covered it with a test, and also updated the issue title. I also took the opportunity to rebase the MR.

restored IS

I think to solve #48.1 we need to drop the decorator - instead move the autowiring discovery to the factory, and call it as a kind of factory-preparation step that follows discovery and alter.

Fixed some more tests. But we have some interesting cases to solve:

1. ContentModerationHooks::actionInfoAlter() swaps the class of two action plugins. But these have different constructor signatures and factory methods, so we need to ideally discover constructor after alter hooks have run. But maybe someone also wants to alter the autowiring metadata?
2. NavigationMenuBlock extends SystemMenuBlock but passes in navigation.menu_tree which is an alternative implementation of MenuLinkTreeInterface. This must be explicitly declared; I think some way of opting in plugins is the only way to solve this.

I added patch #39 to the MR, then made some initial fixes and re-exported config.

None of the existing patches are against the contrib module.

At this point, I have no plans to merge this change.

I've updated the title & description in 📌 Document that URL redirects work from node/{nid} but not alias Needs work to indicate that the maintainers do not plan to support URL aliases. Work can continue there. This issue can probably be closed as "won't fix."

The new numbers look reasonable to me

All the tests are green again. I'm setting this back to Needs Review instead of RTBC only because this wasn't a simple rebase this time. I had to make adjustments to the expected values in the StandardPerformanceTest. They're probably innocuous, but someone should look them over.

The view does not show any results if the forums do not have any containers as parents. Containers are not required, so the Parent view relationship should not be required.

I can confirm that patch is working, we are using it on the project. We are using exactly #263 🐛 Facets with AJAX not working in most of situations Needs review patch.

I could help you out if you want. Let me know here or contact me in slack :)

I'm going to have to iteratively update the StandardPerformanceTest to account for the changes to the cache as a result of the bug fix. Because for some reason my local environment doesn't want to run FunctionalJavascript tests today so that I can do this in one commit.

shouldn't the values be merged? or are we sure the GET data is always more up to date?

Conflicts with ✨ Add a mapping target to media field Needs review

Any updates?

Okay, this also appears for regular wrapped terms:
<strong>Hochwertige Materialien:</strong> Massivholz, MDF oder Aluminium für einen stabilen Halt.

The result is that "Massivholz" is linked, but now missing its preceding space:
Hochwertige Materialien:Massivholz, MDF oder Aluminium für einen stabilen Halt.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Adding postponed to the remaining tasks

ok sure Claudiu, I'll gladly take it, please assign me as co-maintainer with sufficient privileges. Thanks for that link, I can likely backport the ck5_block_embed widget into embed_block quite easily .

I have a module called access_unpublished_linked_nodes that is depending on embed_block , so there is reason for me to continue using this and building on it.
There's also entity_embed , with that said, the markup provided by embed_block is simpler than what entity_embed does.

@joseph.olstad you may want to consider https://www.drupal.org/project/ck5_block_embed → . Also, I'm not using anymore this module and I would be happy to you

If someone is ambitious, we'll need a new branch for the ck5 version of this.

Ah we need return typehints...

Need the patch for a D11 site.

Looks good!

Here is a link to the discussion referenced in 66: https://drupal.slack.com/archives/C079NQPQUEN/p1737757941674099
Also had a quick discussion about how this deprecation is a bit weird cause the values are automatically added. Calling the service is to preserve the current behavior even thought it shouldn't be strictly necessary.

There's AccessTest that probably can be expanded

Left comments but still appears issue summary needs updating.

Minor improvement: The preview remove button form #name was not always unique.

https://www.drupal.org/project/drupal/issues/2899338#comment-15774058 🐛 Wrong render of HTML from label when field is shared across languages in error message Needs work

It appears that this issue has been resolved in the latest version of Drupal 11.

The MR branch was pretty old, so I rebased it on 11.x and added patch #43 on top. Fixed linting and default tests. The non-default tests are failing for unrelated reasons, so think we can ignore those.

It's quite critical issue. In my case: on any change in facet all assets are loaded second time on the page.

There are notes in MR that need to be taken into account

Switch to MR

Running the reflection scan on possibly thousands of plugin definitions where only a fraction of them make use of it looks like a disproportionate resource overhead. Why not handling this similar to Annotations -> Attributes transition? For sure once autowiring is possible, more and more plugin implementations will adapt, but it's a matter of months if not years to come. I'd introduce an Autowire interface for plugins similar as ContainerFactoryPluginInterface to quickly identify which plugin is making use of it. For comparison, services also need an additional autowire: true declaration in order to make use of it. And there may be way more plugin definitions than service definitions around.

For the inheritance problem:
I just found out that we can put attributes on variadic parameters :)

class B {
  public function __construct(
    protected readonly ServiceForB $serviceForB,
  ) {}
}

class C extends B {
  public function __construct(
    protected readonly ServiceForC $serviceForC,
    #[AutowireParentArgs]
    mixed ...$parent_args,
  ) {
    parent::__construct(...$parent_args);
  }
}

If we could create this attribute or find out that it already exists somewhere...

This was mentioned in #11 📌 Allow plugin service wiring via constructor parameter attributes Needs work , but it's been common practice to use ::create() in plugin classes that extend other plugin classes, to avoid issues if the parent constructor's signature changes. I think before deprecating ::create(), at least the suggestion from #12 📌 Allow plugin service wiring via constructor parameter attributes Needs work to accommodate autowired setter injection (presumably with the use of the the \Symfony\Contracts\Service\Attribute\Required atribute) should be done in a follow up.

Restoring status.

Think this needs steps to reproduce that doesn't include a contrib profile. If steps can't be provided think this should be closed.

There's a newer issue that has better code on it so closing a dupe of ✨ Add ignore on duplicate key to insert query Needs work

haha yes "postponed" status threw us off. We still need this with our D11.1.1 upgrade

Automatically closed - issue fixed for 2 weeks with no activity.

We can't just delete ::create() straight away, because a subclass might be calling it. But for plugins that can be autowired, what about flagging ::create() as #[\Deprecated]? This would indicate both that we intend to delete it in the next major (and so we don't need to notify directly that autowiring is available) but any subclasses *should* be notified because it will break in future? Subclasses can then also implement the attribute themselves, or just convert to autowiring.

This is a great upgrade and fix at the same time, thank you! The fix itself still applied to the current version (2.13.0). What's left to get this committed?

I have conducted a detailed analysis of existing deprecation notices of methods in the codebase.

There is a pattern evident in the content of every existing notice. They contain two separate types of information.

1. A statement that the method is removed.
2. Further information

The further information states one and one only of the following::

• There is no replacement
• The method or technique or enum that should now be used instead

The MR I encountered contained 'a statement that the method is removed.' It did not contain 'further information'. So I added the 'further information'. This 'further information' I have drawn attention to in my new comments. I am not sure it is correct.

Added comments to the MR.

RE: #27

@oily I was just seeking confirmation and discussing it with you. No worries at all—

Oh, good. So if i write a comment directed as @smustgrave stating that your MR should be removed and a new one created and you keep creating MR's that need to be replace, you would be cool with that?

The fact is that someone is going to need to find out why this method is being deprecated. And decide whether it is because it is 'Not needed anymore.' or 'the method has no replacement' because every single deprecation notice in the core codebase of this kind contains such a comment. Or else, they need to state why all the other deprecation notices are including unnecessary detail.

RE: #22

@smustgrave Can you please review the last commits? It seems these changes might not be necessary, as this method is not used anywhere.

In that case, I suggest that this notice I have just found in the core codebase contains the message that we need in your MR. (I grepped and found 2x notices stating 'Not needed anymore..')

* @deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Not
* needed any more.

I suggest that you update the MR and replace my additional text with 'Not needed any more.'

@oily I was just seeking confirmation and discussing it with you. No worries at all—let’s see what others think about this. I’m okay with their perspective, and perhaps I was mistaken. No problem, we can focus on another ticket instead of continuing the discussion here.

RE: #25

@oily I was referring to this ticket: #3497796.

As I stated in #23 issue 3497796.is not related to this issue as a parent, child or in any way at all.

If you want to start a fight about 'you did this on issue arandomticket, and you did that on issue anotherrandomissue I am not interested!

But since you seem to want to knock this back to Needs to Review or is is Needs Work go ahead!

@oily I was referring to this ticket: # 3497796 📌 Add return type to hook_file_download implementations Active . The comment update happened after the issue reached RTBC, and that is not the scope of the ticket and unnecessary.

I just meant that since the issue is already in RTBC status, if any changes are needed, please feel free to add a comment. I also want to ensure the issue is completed properly, I am trying to complete the issue what i have started. That’s why I am making this request.

@shahini_jha Perhaps you should look at what others are saying about me:

I also did some minor edits (mostly just formatting) on the CR, and agree that looks really good.

@shalini_jha RE #22

I noticed that @oily has made similar changes in another ticket, which also seem unnecessary.

We are dealing with this issue unless that one is related? What I do in other tickets is immaterial. I work extremely hard on behalf of drupal.org on a great many different tickets. You fail to mention that at #19 I have updated the IS.

Search existing deprecation notices in the core codebase and you'll find it very normal to state the method being replaced or that none is being replaced.

Re-rolling patch #42 for 11.1.1

@smustgrave Can you please review the last commits? It seems these changes might not be necessary. I noticed that @oily has made similar changes in another ticket, which also seem unnecessary.

@oily, if you believe this change is required, please add a comment. I’m already working on this ticket, I will address.

Pipeline is green.

Following #40 I've had an idea for a while that I've now managed to test out, and it's looking quite promising. We can do the reflection at discovery time and store the set of required services for plugins in the plugin definition, then use that metadata in the plugin factory to skip the create() method. MR!11031 has a prototype of this.

Added in deprecation notice that the method has no replacement. I assume it does not. But if it does have a replacement we need to state what it is.

tests are now failing

Change appears fine to me.

Same question here for D11.1 core.

php core/scripts/drupal generate-theme mycustomtheme --name MyCustomTheme --starterkit olivero
[ERROR] Theme source theme olivero is not a valid starter kit.

Just FYI: I updated the MR branch because the patch wasn't applying. It was only after the fact that I realized the patch doesn't apply to version 4.0.0. It did apply to the latest 4.0.x branch.

N.B. this is mainly a problem because code like HtmlResponseAttachmentsProcessor, CssCollectionOptimizerLazy, and AjaxBasePageNegotiator all use code like

  $this->requestStack->getCurrentRequest()->get('ajax_page_state')

and Request::get() checks attributes, query, and request in that order.