🇮🇳India @mohit_aghera

mohit_aghera → created an issue.

jjchinquist → credited mohit_aghera → .

smustgrave → credited mohit_aghera → .

Fixed all the phpstan issues.
Resolved merge conflicts and removed changes related to views as this wasn't necessary.
There is one more failure in the CI, I'm going through that.

I've hidden the patches in favor of MR approach.

I evaluated two solutions for the issue.

1. Modify/cleanup the views in hook_modules_uninstalled hook
2. Implement a new module uninstall validator for the display handler plugins Comment #19

Modify/cleanup the views in hook_modules_uninstalled hook
- I feel this might not work in this case.
Reason is, that we can't fetch the list of all the display handler plugins provided by the module being uninstalled.
In the uninstall() function, this hook is the last one to get called https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Since plugin ID or anything won't be available here, we can't identify which display handler plugins we want to unset from views config.

I evaluated another option to check the plugin by doing namespace-based lookup, however, that seemed a bit hacky to me.

Considering that, it might not be straightforward to update all the views and unset the display handler plugins.
We can certainly disable the plugin in views > Advanced config settings.

Implement a new module uninstall validator for the display handler plugins Comment #19
This worked fine and prevented module being uninstalled.
I added on validator plugin along with kernel test to validate the exception.
This seems to be working fine.

I feel at this point we should go ahead with the approach where we display a warning rather than silently updating the views config.

mohit_aghera → made their first commit to this issue’s fork.

@freelock, thanks for providing the detailed steps to reproduce.
I'll update the tests and will update the PR.

Can we close the other one and keep continue working on this issue itself.
We can carry steps to reproduce.

I did tried to reproduce the issue based on steps mentioned in issue summary.
It seems issue doesn't exist anymore.

I even tried to follow based on step #23 and it seems to be working fine.
I created the view added in patch #16 and added the block in the layout builder for user entity.
It worked fine.

@possibri, @laurentwh
If you noticed the issues, please provide a test view or something I can use to reproduce the issue.

We probably need updated steps to reproduce or see if it has been addressed in recent 11.x cycle.

I could reproduce the issue with the steps mentioned in the issue summary.
We can replicate in vanilla Drupal with just a path module of Drupal core.

I've updated the PR with the latest 11.x and made a little bit of refactoring code improvement fixes.

There are a bunch of test case failures, I'm working on those.
Hiding other patches since MR is the preferred approach.

@ckng
I see you are following the correct steps.
Maybe some other module might affect the outcome?

mohit_aghera → made their first commit to this issue’s fork.

Hi @xjm
I've fixed the following issues from comment #19

• Point 1
• Point 2

Comment is updated for both the cases.

Regarding point 3:
- I've added the new test that validates both the points.
Fixed few more things in the MR

About the custom_url option:
- Primarily it is visible on the block plugin display and other non-page display.
So I believe we should hide it from the page display?

Probably we can use the inputs from the sub-system maintainer.
This might cause confusion to people as `More` link on the page display might not make sense.

I notice that there is no option to configure the link, it just allows to enable/disable link.
Which is correct if we want to keep option.

I'm happy to walk you through the views setup, feel free to ping me in #bugsmash.

Regarding #20
I agree with you for this one.
We can disable it from the page display and keep it active for block display.

Hiding the patches since we have a MR now.
Keeping issue in needs works since there are a few unrelated test failures that I'm looking into it.

Tests added for permissions https://git.drupalcode.org/project/webform_revision_ui/-/merge_requests/....
PR is green now.

mohit_aghera → changed the visibility of the branch project-update-bot-only to active.

mohit_aghera → changed the visibility of the branch project-update-bot-only to hidden.

Thanks for the rebase @mrshowerman

I've fixed the phpstan issues and PR is now green.

I think we are good to get another round of review since all the issues are addressed on the PR

mohit_aghera → changed the visibility of the branch 3107993-3x-template-not-found to active.

mohit_aghera → changed the visibility of the branch 3107993-3x-template-not-found to hidden.

mohit_aghera → created an issue.

I've addressed the feedback in #40
I think node tests aren't required as such.

Fixed a few more things in the PR
- convert it to use OOP hooks
- Fixed a few things in tests.
- Tests seems green now.

Hiding all the patches in favour of MR.

mohit_aghera → made their first commit to this issue’s fork.

@larowlan All the feedback mentioned in the PR is fixed now.
PR is green, I think it is ready for another round of review.

Adding patch for the 10.3.x release cycle since current 11.x patch is not cleanly applied on 10.3.x.

I also noticed this issue while working on the project.
Drupal core: 10.3.10
Issue was noticed on layout builder edit form where core/components.navigation--toolbar-button library was added twice.

Upon debugging, I noticed that it is present twice in ajax_page_state as well.

I think we should apply array_unique when we push the libraries to ajax_page_state so we always have unique libraries in the ajax_page_state.

I've added a PR with the fix.

mohit_aghera → made their first commit to this issue’s fork.

dan2k3k4 → credited mohit_aghera → .

I volunteered Room 2 "The Drupal API Client"

I monitored "Mastering the Art of the Agency Pitch"

Monitored "Jumpstart your Drupal contribution with Bug Smash Initiative"

dan2k3k4 → credited mohit_aghera → .

mohit_aghera → made their first commit to this issue’s fork.

Me and @alexpott had a chat during the DrupalCon Barcelona code sprint.
This issue needs issue summary update.

Primarily we are looking for some system that provides the conform for the recipe.
Intention is to validate that the site already has existing features that the recipe is already providing.

This system should be able to validate that current system state has all the necessary configurations applied.

mohit_aghera → created an issue.

kristen pol → credited mohit_aghera → .

Kristen Pol → credited mohit_aghera → .

I've sent a proposal via Google Form.

I've sent a proposal via Google form.

kim.pepper → credited mohit_aghera → .

mohit_aghera → made their first commit to this issue’s fork.

Addressed the feedback in #138
I'm slightly confused about changes proposed in #139 comment.
I have updated the change record screenshots so it is more useful.

I did tried to fix the feedback mentioned in the #138
I've attached a few screenshots about how it looks like on the backend.

For now, i've pushed the changes related to modified files.
I'll keep an eye on tests and will address additional failures.

Hiding all the old patches and files to prevent the confusion.

mohit_aghera → made their first commit to this issue’s fork.

- All three points mentioned by @acbramley is fixed by @maskedjellybean
- Issue summary is updated and PR is rebased with latest 11.x.
- Merge conflict is resolved.
- Hiding older patches in favour of PR based approach.
- I think it is ready for moving to needs review.

mohit_aghera → created an issue.

mohit_aghera → made their first commit to this issue’s fork.

larowlan → credited mohit_aghera → .

@Mingsong can you please update the PR with latest 11.x and change the destination for 11.x
I've pushed the fix on the branch.
There seems one test case failure, however this is failing on 11.x head as well (on my local).
So not sure if this issue's change is the root cause of this one.

Keeping this in needs work until we update the PR and pipeline is green.

mohit_aghera → made their first commit to this issue’s fork.

mohit_aghera → made their first commit to this issue’s fork.

Fixed the PR feedback.

Fixed the feedback.

@louis-cuny
Marking this as postponed since it needs more information.
Based on snippet given, it seems that cachable dependencies are not given.

Feel free to reopen if you feel it is valid.

Tests are green, back to needs review

mohit_aghera → made their first commit to this issue’s fork.

@catch
I added that initially and my intention was to validate two things.

1. Validate the deprecation message being displayed
2. I should be able use and invoke the older plugins with different constructor argument
   

   $plugin = \Drupal::service('plugin.manager.views.argument')->createInstance('taxonomy_views_argument_test', []);
       $this->assertInstanceOf(TaxonomyViewsArgumentTest::class, $plugin);

Happy to remove the test case entirely or just expect deprecation only.
Let me know your thoughts.

I agree with @efrainh's comment in #13
This doesn't seems to be reproducible in latest 11.x.
I even manually tried in 10.2.x branch and doesn't seem to be reproducible there either.

To confirm that, I wrote one simple test case based on steps to reproduce.
Tests are coming green, so probably issue doesn't exist anymore or we need proper steps to reproduce the issue.

One can refer this PR https://git.drupalcode.org/project/drupal/-/merge_requests/7058 for the confirmation of test.

Updated branch with latest 11.x head since test case failures were related to 11.x HEAD only.
Keeping it to RTBC, will check if there are any failures.

mohit_aghera → made their first commit to this issue’s fork.

Based on @Berdir's feedback in #29,
I've added a PR that adds permissions for `view label` operation.

I think we need some inputs from sub system maintainer about which is the correct approach.
I feel that `view label` access is more safe and restricted.

Updated the issue summary and mentioned that we need consensus about the solution.
Hiding other patches since #37 seems another valid implementation and passing test cases as well.

@adwivedi008, can you please be sure that we need to fix the issues?

Reason is comment in #100is saying that patch is no longer applied to the core.
In this case, you need to merge latest 11.x to the branch.
I tried that and it seemed to be working fine.

dan2k3k4 → credited mohit_aghera → .

Change record created. Back to RTBC

@alexpott do we need a change record for this one?

larowlan → credited mohit_aghera → .

@penyaskito
I'm not able to reproduce the issue.
I've attached one test-only patch which is passing on local against latest 11.x.
I haven't created new branch and fork since I'm not quite sure of this issue is still valid.

Can you please check the test-only patch and see if this is same as what has been given in #2

Hi @Anybody
I think this issue persists in 10.2.x release cycle.
Two issues are already fixed related to that in 10.3.x cycle.
https://www.drupal.org/project/drupal/issues/3416304 🐛 Javascript warning from content language and translation page Fixed
https://www.drupal.org/project/drupal/issues/3414415 🐛 Content language and translation AJAX expansion is backwards Fixed

I think we should cherry-pick those two commits in 10.2.x release cycle as well.
Since 10.2.x is still in active support, it should be easy to cherry pick those.

I even tested 10.3.x and 11.x and those seems works well.

@sakthi_dev, I think patch is already getting applied on 11.x
We probably don't need to re-roll the patch.
Can you please upload the re-roll diff, if possible.

I can confirm that MR #17 works for me as well.
Drupal core 10.1.x and editor_advanced_link: 2.2.4

I noticed the same observations mentionedin #25

Addressed the PR feedback.
I think https://git.drupalcode.org/project/drupal/-/merge_requests/6625#note_276326 would need some verification while doing review.
I'm somewhat confused about that.

Other point seems clear.

I noticed that both the changes has been addressed.
Moving back to RTBC since tests are green as well.

@smustgrave, I think we need to spin up a new branch since you've mentioned that new changes should go into. 2.0.x
We can update the destination for the PR once it is done.

mohit_aghera → made their first commit to this issue’s fork.

Note: This is the different approach compared to all the uploaded patches. So be cautions if you are replacing this with your existing patches. This is based on comms in #80and #82

I've refactored the implementation based on discussion from #80 #82 and #83

Updates on #43
- Fixed point 1 and point 3.
Removed new test for validating both the permissions and removed changes in edit callback.
Reverted un-necessary changes made to the test callbacks.
- Reverted point 1.

I've ignored changes done in-between #50 to #69 since all are removing existing test cases and changing things in node handler.

#70
I'm somewhat confused about content owned by user 0.
I've added the changes and test case changes in commit. I think we can refactor it depending on the feedback.

mohit_aghera → changed the visibility of the branch 2845144-user-cant-reference to hidden.

Note: BC approach to handle annotations and attributes in MigratePluginManager class is being evaluated in https://www.drupal.org/project/drupal/issues/3424509 📌 Update MigratePluginManager to include both attribute and annotation class Fixed

We should revisit and rebase this PR after this is resolved.

Note: BC approach to handle annotations and attributes in MigratePluginManager class is being evaluated in https://www.drupal.org/project/drupal/issues/3424509 📌 Update MigratePluginManager to include both attribute and annotation class Fixed

We should revisit and rebase this PR after this is resolved.

Note: BC approach to handle annotations and attributes in MigratePluginManager class is being evaluated in https://www.drupal.org/project/drupal/issues/3424509 📌 Update MigratePluginManager to include both attribute and annotation class Fixed

We should revisit and rebase this PR after this is resolved.

Note: BC approach to handle annotations and attributes in MigratePluginManager class is being evaluated in https://www.drupal.org/project/drupal/issues/3424509 📌 Update MigratePluginManager to include both attribute and annotation class Fixed

We should revisit and rebase this PR after this is resolved.

- Added the test case for the another ViewsArgument plugin.
- Tests are passing on local as well as CI
- Unusual failure form nightwatch tests, triggered the job again.

Working on the test coverage for the second plugin.