🇮🇳India @mohit_aghera

Updated code to use the config updater service.
There were unrelated functional javascript test failures. Triggered the job again.

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

Attending

mohit_aghera → created an issue.

mohit_aghera → created an issue.

mohit_aghera → created an issue.

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

mohit_aghera → changed the visibility of the branch 2637808-languageformatter to hidden.

mohit_aghera → created an issue. See original summary → .

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.