🇮🇳India @manish-31

MR updated! Added new key for content security policy in tests. Needs review.

manish-31 → made their first commit to this issue’s fork.

I have raised MR to add a checkbox to enable/disable credit and copyright section in theme settings.

Needs review.

manish-31 → made their first commit to this issue’s fork.

The MR patch applies successfully without any warnings now.

Reviewed th Readme.md file has been updated as per the Readme.md standards.

Verified the help page contains all the necessary information and documentation links. Attaching Screenshot for reference.

Marking this RTBC.

Thanks!

Hi @bhaveshdas,

The module does not contain any configuration page so we don't need "Configure" link for the module.

I believe the route name paragraph_locator.settings should be changed to avoid confusion as it handles the deletion of paragraphs.

Hence, closing this issue.

PHPUnit Tests fixed. Needs review!
Thanks!

manish-31 → made their first commit to this issue’s fork.

Yes @mstrelan, IMO it would be better.
Added nullable return type. Needs review. Thanks!

manish-31 → made their first commit to this issue’s fork.

MR raised for the fix. Needs review. Thanks!

manish-31 → created an issue.

MR raised, needs review. Thanks!

manish-31 → made their first commit to this issue’s fork.

Resolved. Needs review.

Installation errors fixed, needs review!

Raised MR against 2.0.x-dev branch. Needs review.

MR contains -
- Fixes in the patch #4 required in 2.0.x-dev
- Test coverage for this change
- hook_update()

Will also need upgrade path.

@smustgrave I am assuming by this you mean update hook for default config. Please let me know if you refer to anything else. Thanks!

manish-31 → made their first commit to this issue’s fork.

MR rebased and fixed new PHPCS issues. @riddhi.addweb Please review now you should be able to apply the MR patch.

manish-31 → made their first commit to this issue’s fork.

MR rebased for conflict resolution.

manish-31 → made their first commit to this issue’s fork.

MR raised to replace deprecated code, needs review.

manish-31 → created an issue.

I have added test for label length option.

Add hook_update to set the fault value
Add yml to config/install with default values.

@VladimirAus Do we really need default values in yml under config/install as we are using dynamic config settings depending on the entity bundle. I am not sure how to implement this keeping this in NW.

Patch #2 works as expected but it trims the label in the hook_entity_presave() once it is generated and resets the label. It should be done when label is generated in setLabel() method in AutoEntityLabelManager class.

I am working on this, will be raising an MR for this soon.

manish-31 → made their first commit to this issue’s fork.

Patch #2 did not work for me, in my case there was no value in $view->display_handler->options['header']. I believe $view->display_handler->getOption('header') would be a good choice and safe option.

Opened MR that includes test coverage for hide header option and the above change. Needs review. Thanks!

manish-31 → made their first commit to this issue’s fork.

@kopeboy I have raised MR to add configuration to configure default publishing options. Please review.

Not adding hook_update() for new configs as we have default case handled in code already.

manish-31 → made their first commit to this issue’s fork.

Hi @rsvelko,
Thanks for merging the MR.

I noticed that I wasn't credited for resolving this issue. I'm not sure why that happened. Is there something I'm missing?

MR rebased, Gitlab CI ran PHPCS validation ran successfully. Needs review.

manish-31 → made their first commit to this issue’s fork.

Opened an MR.
Added tests for this change and minor coding standard fixed from patch #36. Needs review.

manish-31 → made their first commit to this issue’s fork.

Reviewed the MR patch, it works as expected.Added hook_update(), needs review.

manish-31 → made their first commit to this issue’s fork.

Added log for redirect delete form. Needs review.

manish-31 → made their first commit to this issue’s fork.

manish-31 → made their first commit to this issue’s fork.

Resolved conflicts and fixed all the remaining PHPCS issues. Needs review.

manish-31 → changed the visibility of the branch 3372210-fix-the-issues to active.

manish-31 → changed the visibility of the branch 3372210-fix-the-issues to hidden.

I believe it's not a good idea to change the feature completely. Instead, we should make it configurable by adding an option to configure the toggle behaviour while keeping the existing feature as is.

Updating status to NW.

Raised MR to fix all PHPCS issues. Needs review.

manish-31 → made their first commit to this issue’s fork.

MR has been updated, fixed PHPUnit tests errors. Needs review.

manish-31 → made their first commit to this issue’s fork.

Resolved merge conflicts. Needs review.

manish-31 → made their first commit to this issue’s fork.

I have fixed the remaining PHPCS errors. Needs review. Thanks!

manish-31 → made their first commit to this issue’s fork.

I have also confirmed the MR resolves all the PHPCS warnings.

RTBC +1

Updating ticket status from NW to RTBC.

MR patch from Merge request !2 applies successfully and resolves all the PHPCS warnings. Attaching the screenshots for the same.

RTBC +1

Hi @diwakar

Help page looks better now.

I got some trailing whitespaces warnings while applying the patch could you please fix those?

182: trailing whitespace.
      $output .= '<ol> 
187: trailing whitespace.
      <li> Set permissions for commenting as per usual from 
203: trailing whitespace.
      
warning: 3 lines add whitespace errors.

Also, please consider below improvements
- Remove colons from paragraph headings.

- Provide absolute URLs to the configuratin/permission pages.

Rebased MR and resolved Merge conflicts.

The MR patch from #6 works perfectly.. I have confirmed there is no "access denied" error on registration/login after applying this patch.

Attaching the screenshots of registration and login.

Marking this as RTBC. Thanks!

I tested the patch from MR but it all PHPCS warnings are not fixed. Still some warnings in stripe_integration.module, StripeController.php and stripe_integration.info.yml files.

manish-31 → made their first commit to this issue’s fork.

I have tested the MR patch, the reported issue is fixed.

However, there still one error -

Error: Class "Drupal\tmgmt_file\Format\FormatManager" not found in Drupal\Component\DependencyInjection\Container->createService() (line 259 of /app/web/core/lib/Drupal/Component/DependencyInjection/Container.php) #0 /app/web/core/lib/Drupal/Component/DependencyInjection/Container.php(177): Drupal\Component\DependencyInjection\Container->createService(Array, 'plugin.manager....')
#1 /app/web/modules/contrib/wordsonline_connector/src/Plugin/tmgmt/Translator/WordsOnlineTranslator.php(143): Drupal\Component\DependencyInjection\Container->get('plugin.manager....')
#2 /app/web/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php(21): Drupal\wordsonline_connector\Plugin\tmgmt\Translator\WordsOnlineTranslator::create(Object(Drupal\Core\DependencyInjection\Container), Array, 'wordsonline', Array)
#3 /app/web/core/lib/Drupal/Component/Plugin/PluginManagerBase.php(83): Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('wordsonline', Array)
#4 /app/web/modules/contrib/tmgmt/src/Entity/Translator.php(270): Drupal\Component\Plugin\PluginManagerBase->createInstance('wordsonline')
#5 /app/web/modules/contrib/tmgmt/tmgmt.module(491): Drupal\tmgmt\Entity\Translator->getPlugin()
#6 /app/web/modules/contrib/tmgmt/tmgmt.module(66): tmgmt_translator_auto_create(Array)
#7 [internal function]: tmgmt_modules_installed(Array, false)
#8 /app/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(409): call_user_func_array(Object(Closure), Array)
#9 /app/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(388): Drupal\Core\Extension\ModuleHandler->Drupal\Core\Extension\{closure}(Object(Closure), 'tmgmt')
#10 /app/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(416): Drupal\Core\Extension\ModuleHandler->invokeAllWith('modules_install...', Object(Closure))
#11 /app/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php(388): Drupal\Core\Extension\ModuleHandler->invokeAll('modules_install...', Array)
#12 /app/web/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php(83): Drupal\Core\Extension\ModuleInstaller->install(Array, true)
#13 /app/vendor/drush/drush/src/Commands/pm/PmCommands.php(102): Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array, true)
#14 [internal function]: Drush\Commands\pm\PmCommands->install(Array, Array)
#15 /app/vendor/consolidation/annotated-command/src/CommandProcessor.php(276): call_user_func_array(Array, Array)
#16 /app/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
#17 /app/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#18 /app/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(391): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#19 /app/vendor/symfony/console/Command/Command.php(326): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#20 /app/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#21 /app/vendor/symfony/console/Application.php(324): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#22 /app/vendor/symfony/console/Application.php(175): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#23 /app/vendor/drush/drush/src/Runtime/Runtime.php(110): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#24 /app/vendor/drush/drush/src/Runtime/Runtime.php(40): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
#25 /app/vendor/drush/drush/drush.php(139): Drush\Runtime\Runtime->run(Array)
#26 /app/vendor/drush/drush/drush(4): require('/app/vendor/dru...')
#27 /app/vendor/bin/drush(120): include('/app/vendor/dru...')
#28 {main}

I think above error can be tracked in a separate issue once this is fixed, so marking this as RTBC. Thanks!

I have applied the MR patch and tested PHPCS warnings. No warnings/errors on PHPCS scan.

Attaching screenshot and marking RTBC. Thanks!

I have tested the patch and it is working as expected. Attaching Screenshots and marking RTBC.

I have confirmed that this error does not occur after applying this MR patch. Marking this as RTBC.

Hi @stBorchert and @smustgrave

I have rebased the existing MR MR !8199 and dropped unnecessary commits from the MR. Needs review. Thanks!

Additionally, I made a minor change by removing Database::connection() and instead using the $connection variable from the extended class.

manish-31 → made their first commit to this issue’s fork.

I have raised Merge Request for the patch from #2 including the feedback fixes from #4. Needs review.

I have confirmed that help page does not throw this error now.

manish-31 → made their first commit to this issue’s fork.

I have opened a Merge Request to fix the warning - "Deprecated function: explode()."

Also, while reproducing this warning I found that for the first time until we save style settings we get another error - "TypeError: array_filter(): Argument #1 ($array) must be of type array, null given in array_filter() (line 372 of modules/contrib/views_bootstrap/views_bootstrap.theme.inc)." which gets fixed once we save style settings.

This MR contains fix for this too. Needs review. Thanks!

manish-31 → made their first commit to this issue’s fork.

I have opened a Merge Request to fix this issue. Needs review.

The issue occurs because when we don't allow link text in link field it is no longer a "fieldset" so it skips the expected condition for link text.

Attached Screenshot after fix.

manish-31 → made their first commit to this issue’s fork.

I have rebased the MR, resolved conflicts from /core/modules/system/tests/src/Functional/Menu/LocalTasksTest.php and /core/modules/system/src/Controller/AssetControllerBase.php

Also, added some more fixes as per the comment #62.

manish-31 → made their first commit to this issue’s fork.

Raised MR to add ['target' => '_blank'] attribute to all the external URLs. Needs review. Thanks!

manish-31 → created an issue.

Added reroll patch for the patch from #32. Needs review.

Shall we close the MR!4 and raise a new MR or push the changes in the same MR?

manish-31 → made their first commit to this issue’s fork.

Hi @sarwan_verma,

IMO we should not fix this by just adding an isset() check the warning occurs only when Real AES module is installed and the checkbox gets removed from the checklist options. Which looks wrong to me.

As in other cases, we are showing the project link if a module is missing and also showing settings/configuration link if the module is installed we should replicate the same behaviour here.

I have raised an MR to implement the same. Attached Screenshots of the UI for both cases Real AES enabled and disabled. Needs review.

manish-31 → made their first commit to this issue’s fork.

Added Multi-line function declarations as Issue #3295249 → has been fixed.

@mstrelan have we not finalized the deprecation message we are going to trigger?

manish-31 → made their first commit to this issue’s fork.

Hi @kenyoOwen,

I have already followed the https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... → format while replacing the file. Please check the MR and point out the issue.

Marking Needs review. Thanks!

I have reviewed and tested this patch https://git.drupalcode.org/project/google_analytics/-/merge_requests/36...., applies successfully and fixes the reported error.

To replicate the error I have set the value of "account" in google_analytics.setting config to NULL.

I have raised MR to fix PHPCS warnings. Needs review.

manish-31 → made their first commit to this issue’s fork.

This MR introduced an error when running the Test ParagraphListingTest.

Not re-opening the issue as this has been fixed with the MR changes done in https://www.drupal.org/project/paragraphs_admin/issues/3443579 ✨ Paragraph delete should have confirm form Needs work (under review).

Test output -

www-data@c06cb4a3c870:/app/web/core$ ../../vendor/bin/phpunit ../modules/contrib/paragraphs_admin/tests/src/Functional/ParagraphListingTest.php --debug
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\paragraphs_admin\Functional\ParagraphListingTest
Test 'Drupal\Tests\paragraphs_admin\Functional\ParagraphListingTest::testParagraphListingPages' started
Test 'Drupal\Tests\paragraphs_admin\Functional\ParagraphListingTest::testParagraphListingPages' ended


Time: 00:10.387, Memory: 10.00 MB

There was 1 error:

1) Drupal\Tests\paragraphs_admin\Functional\ParagraphListingTest::testParagraphListingPages
TypeError: array_merge(): Argument #2 must be of type array, Drupal\user\Entity\User given

/app/web/modules/contrib/paragraphs/tests/src/Functional/WidgetLegacy/ParagraphsTestBase.php:89
/app/web/modules/contrib/paragraphs_admin/tests/src/Functional/ParagraphListingTest.php:48
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

ERRORS!
Tests: 1, Assertions: 9, Errors: 1.

I have pushed the required changes in Merge Request !8 suggested by @VladimirAus

MR updates -

• Removed the controller since we are now using entity delete form we no longer need the controller to delete paragraphs.
• Used Drupal\Core\Entity\ContentEntityDeleteForm to build the delete form with confirmation.
• Added new assertions to test the delete functionality as per the new changes.
• I have verified there are no PHPCS warnings now.

Needs review. Thanks!

manish-31 → made their first commit to this issue’s fork.

Raised MR to replace Readme.txt to README.md

Needs review thanks!

manish-31 → made their first commit to this issue’s fork.

Not sure why we have opened two MR's we can close MR !4.

I have updated merge request !6, replaced property_exits() check with isset() to avoid another unnecessary check.

Needs review. Thanks!

manish-31 → made their first commit to this issue’s fork.

Hi @Steven Jones I agree with the suggestion on comment #17 to not use a fallback if authenticated role is also empty and consider it as opt out option.

I have rebased the MR merge request !8 , resolved the conflicts and a minor change in the code to set "destination" query parameter for redirection.

I have verified the MR patch from merge request !8 it works as expected , approach looks good to me. Keeping it in NR status for now for further review.

manish-31 → made their first commit to this issue’s fork.

I have rebased the MR and resolved MR conflicts.

Fixed another issue detected by PHPCS after rebasing the MR. Needs review thanks!

manish-31 → made their first commit to this issue’s fork.

I have updated the MR with the changes required as per the MR feedback. Needs review.

I confirmed that there are no errors when running PHP CodeSniffer now, so I'm updating the issue status to 'Needs Review'.