Account created on 30 October 2017, over 7 years ago
  • Software Engineer at Acquia 
#

Merge Requests

More

Recent comments

🇮🇳India ankitv18

As it is dependent on https://www.drupal.org/project/acquia_optimize/issues/3517839 Add Experience Builder extension Active so I have separate commit: https://git.drupalcode.org/project/acquia_optimize/-/merge_requests/6/di...

🇮🇳India ankitv18

All the changes as a part of this issue is covered ~~ hence moving into review

🇮🇳India ankitv18

MR!29 is against 2.x branch ~~ phpcs pipeline + composer changes are fixed now, hence moving into review.

🇮🇳India ankitv18

ankitv18 changed the visibility of the branch 3528488-fix-the-phpcs-8.x-2.x to hidden.

🇮🇳India ankitv18

ankitv18 changed the visibility of the branch 3528488-fix-phpcs-issue to hidden.

🇮🇳India ankitv18

ankitv18 changed the visibility of the branch 8.x-1.x to hidden.

🇮🇳India ankitv18

Validated the MR!5
Query builder page throwing 404 error

jsonapi_query_builder_update_10001 isn't executed properly

🇮🇳India ankitv18

MR!6 is ready for a review
Pipeline is passing after adding few phpunit and mink dev packages
Also made some tweaks and skipping few tests of Functional JsonApiQueryBuilderTest

🇮🇳India ankitv18

uhhghh... I missed your comment :(
FYI, there are console.error and console.warn also in the react-app directory.
Do we need to remove them also in this ticket?

🇮🇳India ankitv18

ankitv18 made their first commit to this issue’s fork.

🇮🇳India ankitv18

ankitv18 made their first commit to this issue’s fork.

🇮🇳India ankitv18

Provided few suggestion on the MR and also test case is missing for this new drush command.

🇮🇳India ankitv18

Tests are also fixed as a part of this issue ~~ now pipelines are passing, hence moving into NR.

🇮🇳India ankitv18

ankitv18 made their first commit to this issue’s fork.

🇮🇳India ankitv18

Ignore my comment ~~ went through the file then understood the reason of typecasting the numeric value.

Changes looks to be mergeable as pipeline are clean ~~ marking this one RTBC

🇮🇳India ankitv18

thanks @batigolix, Changes looks good ~~ marking this one RTBC

🇮🇳India ankitv18

@cmlara I guess you missed to give credit to the contributors for this issue :)

🇮🇳India ankitv18

I have validated this MR on local with XB ~~ attaching the screenshot for the result.

@mglaman @grasmash is there any remaining work on this FR?

🇮🇳India ankitv18

If changes looks good then we can consider to merge this into 3.1.x also

🇮🇳India ankitv18

Validated the changes on local and now I'm able to get the Quick Scan result without any browser console error ~~ Hence marking this one RTBC

🇮🇳India ankitv18

ankitv18 made their first commit to this issue’s fork.

🇮🇳India ankitv18

Minor suggestion moving that line of change from constructor to Query function would work.

🇮🇳India ankitv18

ankitv18 made their first commit to this issue’s fork.

🇮🇳India ankitv18

Marking this one outdated as this issue isn't replicated with latest release.
Feel free to open new issue if related issue encounter.

🇮🇳India ankitv18

I had tried to reproduce this issue at my local machine with the latest version of Key module along with other contributed modules like workbench_moderation, shield and encrypt on Drupal 11.1.5 application.

Can anyone else give a shot to check whether this issue still exists?

cc: @acbramley @rlhawk @q0rban

🇮🇳India ankitv18

Hey @joseph.olstad
Is this issue still exists as I tried with the below version:
Drupal: 11.1.5
Key: 1.20
smtp: 1.4.0
symfony_mailer: 1.5.0
symfony_mailer_lite: 2.0.2

Below are the screenshot of overriding smtp username with custom test key.

Am I missing something here? could you check with the latest version of modules and update the issue accordingly?

🇮🇳India ankitv18

So I've updated the MR ~~ now it consists changes in composer.json file and hook_update_N (to install acquia_dam).

🇮🇳India ankitv18

We should set a soft dependency, allowing users to uninstall media_acquiadam after successfully migrating assets.

Keeping acquia_dam in the info.yml prevents users from performing the uninstall action.

Also user needs to upgrade on latest version of media_acquiadam so by default they won't get acquia_dam module enabled and However, the migration process requires the acquia_dam module, which users can enable manually through the extend menu.

🇮🇳India ankitv18

Pipeline runs smoothly ~~~ No more warnings are there. Also, changes looks mergeable ~~ Hence moving into RTBC.

🇮🇳India ankitv18

Provided some suggestion ~~ also validated the functionality, it works as expected.

🇮🇳India ankitv18

Please add a hook_update_N to by default enable the Acquia DAM module so that user can see the migration dashboard fully functional.

🇮🇳India ankitv18

Ideally this should be the part of https://www.drupal.org/project/acquia_dam/issues/3515288 🐛 Newly created media type doesn't have media_library entity view display preconfigured. Active or if we are considering handling this separately then I've added the comment on the MR.

🇮🇳India ankitv18

MR merged by Vishal and reviewed by Deepak

🇮🇳India ankitv18

The following files are responsible for handling the sorting and rendering of the asset count for dam_content_overview:

https://git.drupalcode.org/project/acquia_dam/-/blob/1.1.x/src/Plugin/vi...
https://git.drupalcode.org/project/acquia_dam/-/blob/1.1.x/src/Plugin/vi...

I attempted to debug this issue but was unsuccessful. I have attached a screenshot of the Xdebug for reference.

🇮🇳India ankitv18

ankitv18 changed the visibility of the branch 3447628-load-the-config to hidden.

🇮🇳India ankitv18

ankitv18 made their first commit to this issue’s fork.

🇮🇳India ankitv18

ankitv18 changed the visibility of the branch 3422332-fix-gitlab-pipeline to hidden.

🇮🇳India ankitv18

ankitv18 made their first commit to this issue’s fork.

🇮🇳India ankitv18

Please rebase the MR so that CI jobs at least executed properly.
With referring #21 by @adamps if there's already a test then that need to be updated.

🇮🇳India ankitv18

Minor change required on MR.

🇮🇳India ankitv18

RTBC++
Faced similar issue with Drupal 11.1.3 version.
Fatal error: Declaration of Drupal\cors_ui\CorsUiCompilerPass::process(Symfony\Component\DependencyInjection\ContainerBuilder $container) must be compatible with Drupal\Core\DependencyInjection\Compiler\CorsCompilerPass::process(Symfony\Component\DependencyInjection\ContainerBuilder $container): void in /var/www/html/docroot/modules/contrib/cors_ui/src/CorsUiCompilerPass.php on line 18

🇮🇳India ankitv18

I've incorporate the changes with https://git.drupalcode.org/project/acquia_dam/-/merge_requests/113/diffs... (as this MR consists of Field swapping on media_type_presave hook with most of the logic using Asset, AssetDriver and Hook)

🇮🇳India ankitv18

ankitv18 made their first commit to this issue’s fork.

Production build 0.71.5 2024