๐Ÿ‡ฎ๐Ÿ‡ณIndia @kunal.sachdev

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

Merge Requests

More

Recent comments

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

Change the status of CR to be published.

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

Looks good. Just have one minor doubt.

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

Change record created [#3458722].

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

kunal.sachdev โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ณ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

@phenaproxima I think this comment ( #18 ๐Ÿ“Œ Add validation constraints to system.theme Needs work ) was meant for ๐Ÿ“Œ [PP-1] Add validation constraints to core.extension Postponed .

๐Ÿ‡ฎ๐Ÿ‡ณ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

kunal.sachdev โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ณ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

kunal.sachdev โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ณ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

The tests are passing now.

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

kunal.sachdev โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ณ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

Updated CR and IS.

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

Created CR [#3428313]

๐Ÿ‡ฎ๐Ÿ‡ณ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

kunal.sachdev โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ณ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 ๐Ÿ› There is no indication on configuration forms if there are overridden values Needs work : 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 ๐Ÿ› There is no indication on configuration forms if there are overridden values Needs work : 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.

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

Addressed all the feedback. It needs a usability review as mentioned in #90 โ†’

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

All tests are passing now and I think there is no need to postpone this issue anymore.

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

Rechecked it and found that title is also not shown in claro, hence updating the issue summary and title.

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

I have updated the remaining tasks and proposed resolution. I will start working on the remaining tasks.

๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

Tested it locally and works as expected. The screenshots needs to be updated in IS as the field selection is a two step form now.

Production build 0.69.0 2024