Account created on 13 August 2021, over 3 years ago
  • Associate Engineer at Acquia 
#

Merge Requests

More

Recent comments

🇮🇳India kunal.sachdev

kunal.sachdev made their first commit to this issue’s fork.

🇮🇳India kunal.sachdev

The current test failure is in tests/src/Nightwatch/Tests/keyboardTest.js in which there is a part where we are testing that when we are on the categories filter and we click tab then it shifts to next filter drop down i.e security coverage. This test is failing now because in this MR we are rendering the filters as it is which is defined from the backend and not hardcoding them. The previous hardcoding was done in a way such that the filters were displayed in the order :- categories -> security coverage -> maintenance status -> development status but now we accidentally in the backend have the filters defined in the order :- categories ->maintenance status -> security coverage -> development status. For now to fix the failure we can fix the order of the defined filters in backend but I think the major problem is that somewhere in the code we have hard coded the tab index for each filter which needs to be fixed (maybe in a new issue).

🇮🇳India kunal.sachdev

Added test coverage for the changes to ExtensionExistsConstraintValidator. Also we have update path and its test so removing the related issue tags.

🇮🇳India kunal.sachdev

I also tried to use AltLeastOneOf constraint in 📌 Add validation constraints to core.menu.schema.yml Needs work and I have mentioned the problems I faced in https://www.drupal.org/project/drupal/issues/3441434#comment-15686742 📌 Add validation constraints to core.menu.schema.yml Needs work .

🇮🇳India kunal.sachdev

I tried to use AtLeastOneOf constraint here but I faced two problems here :-

  1. AtLeastOf constraint requires an array of constraint objects and we were getting constraint arrays here. So what I did was implemented a plugin class for AtLeastOneOf, and converted the constraint arrays into constraint objects.
  2. RecursiveContextualValidator prevents us from using custom groups. But we want to use AtLeastOneOf constraint there and I think it's validator requires every constraint to have a group. So for that I created this follow-up 📌 Remove the restriction from RecursiveContextualValidator that prevents using custom groups. Needs review .
🇮🇳India kunal.sachdev

Looks good. Just have one minor doubt.

🇮🇳India kunal.sachdev
  • @phenaproxima thinks that ExtensionExists constraint doesn't make a ton of sense for core.extension see ( 3441503-#18 📌 Add validation constraints to system.theme Needs work ) for more details.
  • Also discussed this with @phenaproxima in slack and concluded that if we want to use ExtensionExists constraint for core.extension then ExtensionExistsConstraintValidator should be completely refactored and use the ModuleExtensionList and ThemeExtensionList, which can report on existing-but-not-yet-installed things.
🇮🇳India kunal.sachdev

The test is failing because in RecipeRunner::installTheme() we do

\Drupal::service('theme_installer')->install([$theme]);

and in ThemeInstaller we have

$extension_config
        ->set("theme.$key", 0)
        ->save(TRUE);

which also tries to validate the config and it breaks for ExtensionExistsContraint with error message 'Theme stark is not installed', which is true.

🇮🇳India kunal.sachdev

Most tests are failing because in TestBase setup() we do

$this->container->get('theme_installer')->install(['stark']);

and in ThemeInstaller we have

$extension_config
        ->set("theme.$key", 0)
        ->save(TRUE);

which also tries to validate the config and it breaks for ExtensionExistsContraint with error message 'Theme stark is not installed', which is true. I think we could add a check in ThemeInstaller before saving the key in the config, something like this-

if (!isset($installed_themes[$key])) {
        $extension_config
        ->set("theme.$key", 0)
        ->save(TRUE);
 }

, but I am not really sure about this.

🇮🇳India kunal.sachdev

Looks good, just some minor docs and test changes required.

🇮🇳India kunal.sachdev

For #9 📌 Add validation constraints to system.mail Needs work we already have a constraint

        constraints:
          PluginExists:
            manager: plugin.manager.mail
            interface: 'Drupal\Core\Mail\MailInterface'

to check if the plugin name is valid.

🇮🇳India kunal.sachdev

and about this in

but if I look at the merge request, that file is not updated at all:

#116 Add UUID to sections Needs work in the file LayoutBuilderEntityViewDisplayStorage is not updated but the method LayoutBuilderEntityViewDisplayStorage::mapFromStorageRecords() uses a method \Drupal\layout_builder\Section::fromArray(). which is updated here in the MR.

🇮🇳India kunal.sachdev

It's LayoutBuilderEntityViewDisplayStorage and not LayoutBuilderEntityViewDisplay, corrected the IS.

🇮🇳India kunal.sachdev

@Wim Leers I was referring to #3208766-83 Add UUID to sections Needs work in #109 Add UUID to sections Needs work .

🇮🇳India kunal.sachdev

Yes, I was waiting for someone to answer the first question in https://www.drupal.org/project/drupal/issues/3208766#comment-15354646 Add UUID to sections Needs work before removing the post-update hook. But now I think I'll remove the post-update hook for now and we can add it back if we need it. I am adding the question to the remaining tasks in IS.
Also, updated the IS in general.

🇮🇳India kunal.sachdev

Updated the IS as before screenshots added in #68 📌 Improve menu parent link selection Needs work .

🇮🇳India kunal.sachdev

Yes, if the answer to the first question in https://www.drupal.org/project/drupal/issues/3208766#comment-15354646 Add UUID to sections Needs work is that it's not a concern then we will not need any post-update/update hooks. Hence, marking it again to "Needs review".

🇮🇳India kunal.sachdev

The performance test is failing because this is introducing an additional cache query in core/modules/user/user.module::user_form_process_password_unmask(). I think we can increase the query counts by one to resolve the test failure.

🇮🇳India kunal.sachdev

#20 🐛 Display the page title, even if "0" in olivero Active : Instead of creating a follow-up to do testing in a loop I changed the test in this issue only.

🇮🇳India kunal.sachdev

Tests are passing and all feedback is addressed, but there are 26 unresolved threads and most of them can be resolved.

🇮🇳India kunal.sachdev

#223 🐛 Display status message on configuration forms when there are overridden values Fixed : This is happening because there are two forms ClearCacheForm and PerformanceForm on Performace Page and it shows the status messages of the both the forms on the top of the respective forms. I discussed this with @lauriii and we think it's not a problem, so we can keep it it as it is for now.

🇮🇳India kunal.sachdev

Worked on addressing feedback but there are 26 unresolved threads and most of them can be resolved. Also there is an unrelated CSpell failure in MR.

🇮🇳India kunal.sachdev

Addressed all the feedback. Also there is an unrelated CSpell failure in MR fatal: bad object c574a6e4f0867d27a49a244ee9648d7adfaf9f9b.

🇮🇳India kunal.sachdev

#218 🐛 Display status message on configuration forms when there are overridden values Fixed : No, this not what is expected. It is working like this because there are two forms ClearCacheForm and PerformanceForm and it shows the status message on the top of the PerformanceForm below the ClearCacheForm.

Production build 0.71.5 2024