Account created on 4 January 2013, almost 12 years ago
  • Staff Software Engineer at Acquia 
#

Merge Requests

More

Recent comments

🇮🇳India vishalkhode

Reviewed changes, looks good to me. We can remove temp commit and request for merge. Hence, marking RTBC.

🇮🇳India vishalkhode

Reviewed changes, looks good to me. Hence, RTBC.

🇮🇳India vishalkhode

Reviewed changes, looks good to me. Hence, RTBC.

🇮🇳India vishalkhode

Reviewed changes, looks good to me now. Hence, RTBC.

🇮🇳India vishalkhode

Reviewed MR !49, looks good to me now. Hence, RTBC.

🇮🇳India vishalkhode

Reviewed changes, looks good to me assuming 400 status code returned by Widen DAM only in case of scenarios where assets are being deleted/removed directly on Widen DAM portal.
Hence, RTBC.

🇮🇳India vishalkhode

Thanks everyone for working on this. Fixed in 9ddfc and released as part of 1.4.5 .

🇮🇳India vishalkhode

Hi @gausarts
Thanks for your suggestions, duly noted. I realize that I mistakenly updated the service from library.discovery to library.discovery.collector, which is now deprecated. I believe this would only break in Drupal 12 (which hasn’t been released yet), correct? That’s why we have the review system on drupal.org—to ensure that reviewers can catch these mistakes, suggest changes, and help get them fixed before merging. I agree that mistakes happen, but that’s where the review process comes in to catch and correct them.

The bigger issue here, in my opinion, is not that developers make incorrect changes in their MRs—nobody sets out to do that. The main problem is the absence of CI for this module. If we had CI, it would automatically test the module across all supported Drupal Core versions, flagging any test failures, deprecations, or code standards issues. Without this in place, it’s easy to merge changes that haven’t been fully tested across the different core versions.
Thanks.

🇮🇳India vishalkhode

Thanks @Rajeshreeputra for working on this. The Config Filter module dependency has been removed as it's not required.

🇮🇳India vishalkhode

Mistakenly Drupal 11 changes have been pushed as part of this, hence reverting and will create a new minor release, supporting Drupal 11 and removal of Config Filter module dependency and it'll done as part of 📌 Automated Drupal 11 compatibility fixes for sitestudio_config_management Needs review . Thanks everyone.

🇮🇳India vishalkhode

Thanks @nord102. Fixed and release as part of 3.3.11 .

🇮🇳India vishalkhode

@ok4p1: Yes, we've release by this week.

🇮🇳India vishalkhode

Not able to reproduce the issue with latest release. Followed the steps as mentioned in #25 and also followed the steps that's added in the ticket. Feel free to re-open the ticket, if this issue still occurs and we've the steps to reproduce the issue.
Thanks.

🇮🇳India vishalkhode

Reviewed changes, looks good to me now. Hence, RTBC.

🇮🇳India vishalkhode

There are couple of failures in Next Major CI, which are un-related to this issue, hence, I've not fixed this as part of this ticket. But If needed/requested, I'll fix those here. Hence, moving it to Needs Review.

🇮🇳India vishalkhode

vishalkhode changed the visibility of the branch 3471672-the-module-fails to active.

🇮🇳India vishalkhode

vishalkhode changed the visibility of the branch 3471672-the-module-fails to hidden.

🇮🇳India vishalkhode

@ankitv18 It's working fine now when specifying using the full namespace. The problem/issue seems to me is due to the PHP mocking, because we've created a mock of HttpKernelInterface interface in previous line and I believe the defined() check might referring to the mocked version of the class and it doesn't have the class constant defined. Hence, defined method was returning false.
Rest all changes looks good to me. Hence, RTBC.

🇮🇳India vishalkhode

Overall changes looks good to me. One minor change requested, please take a look. Hence, moving it back.

🇮🇳India vishalkhode

Reviewed changes, looks good to me. Hence, RTBC.

🇮🇳India vishalkhode

Reviewed changes, looks good to me. Hence, RTBC.

🇮🇳India vishalkhode

Making dataProviders to static in PHPUnit tests is must requirement for D11 compatibility. Hence, this needs to fixed first.

🇮🇳India vishalkhode

The Scheduler module has now 2.x-dev dev release available, so we can start working on this. Hence, re-opening this.

🇮🇳India vishalkhode

Ok, so the error was there, because I fixed the error, but didn't notice that the same error was added to ignore in PHPStan baseline file. But now I've updated the baseline file and removed that portion and so everything looks good now. Hence, requesting review.

But now, I can see different PHPStan errors in Current and Next Major Drupal Core version. Ex: https://git.drupalcode.org/issue/scheduler-3467349/-/pipelines/253073.

🇮🇳India vishalkhode

@jonathan1055: Ok, so I tweaked the method formatOutputText() and converted the regular function to an anonymous function (or closure) and assigned to the $formatOutputText variable. This allows us to use it like a regular function within the resetFormDisplayFields method. Because anyways defining a function inside another function or method, is I think generally not a good practice in PHP.

But now, I can see different PHPStan errors in Current and Next Major Drupal Core version. Ex: https://git.drupalcode.org/issue/scheduler-3467349/-/pipelines/253073
But I strongly think, this shouldn't have been caused due to my changes: https://git.drupalcode.org/issue/scheduler-3467349/-/compare/2.x...34673...

🇮🇳India vishalkhode

Further debugging, I found that the code in mglaman/phpstan-drupal extension is causing the issue. When I downgraded the mglaman/phpstan-drupal extension from 1.2.12 to 1.2.11, the error doesn't show up. I think @mglaman can help us here.

🇮🇳India vishalkhode

Hi @jonathan1055
Yes, I've not pushed anything, just got the fork push access and re-ran the job to see if maybe it's due to some issue of CI, but unfortunately it's not, but somehow I'm able to reproduce the issue, following is the stack trace I'm seeing:

-- ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ 
     Error                                                                                                                                                                                                                               
 -- ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ 
     Internal error: Method Drupal\scheduler\SchedulerManager::Drupal\scheduler\formatOutputText() does not exist while analysing file /Users/vishal.khode/drupal11/scheduler/src/SchedulerManager.php                                   
     Post the following stack trace to https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml:                                                                                                                           
     ## phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/ondrejmirtes/better-reflection/src/Reflection/Adapter/ReflectionClass.php(161)                                                          
     #0 /Users/vishal.khode/drupal11/scheduler/vendor/mglaman/phpstan-drupal/src/DeprecatedScope/IgnoreDeprecationsScope.php(31): PHPStan\BetterReflection\Reflection\Adapter\ReflectionClass->getMethod()                               
     #1 /Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan-deprecation-rules/src/Rules/Deprecations/DeprecatedScopeHelper.php(24): mglaman\PHPStanDrupal\DeprecatedScope\IgnoreDeprecationsScope->isScopeDeprecated()         
     #2 /Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan-deprecation-rules/src/Rules/Deprecations/TypeHintDeprecatedInFunctionSignatureRule.php(40): PHPStan\Rules\Deprecations\DeprecatedScopeHelper->isScopeDeprecated()  
     #3 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/FileAnalyser.php(120): PHPStan\Rules\Deprecations\TypeHintDeprecatedInFunctionSignatureRule->processNode()                        
     #4 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Node/ClassStatementsGatherer.php(109): PHPStan\Analyser\FileAnalyser->PHPStan\Analyser\{closure}()                                         
     #5 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(647): PHPStan\Node\ClassStatementsGatherer->__invoke()                                                      
     #6 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(571): PHPStan\Analyser\NodeScopeResolver::PHPStan\Analyser\{closure}()                                      
     #7 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(458): PHPStan\Analyser\NodeScopeResolver->processStmtNode()                                                 
     #8 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(646): PHPStan\Analyser\NodeScopeResolver->processStmtNodes()                                                
     #9 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(458): PHPStan\Analyser\NodeScopeResolver->processStmtNode()                                                 
     #10 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(783): PHPStan\Analyser\NodeScopeResolver->processStmtNodes()                                               
     #11 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(458): PHPStan\Analyser\NodeScopeResolver->processStmtNode()                                                
     #12 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(753): PHPStan\Analyser\NodeScopeResolver->processStmtNodes()                                               
     #13 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/NodeScopeResolver.php(423): PHPStan\Analyser\NodeScopeResolver->processStmtNode()                                                
     #14 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/FileAnalyser.php(179): PHPStan\Analyser\NodeScopeResolver->processNodes()                                                        
     #15 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Command/WorkerCommand.php(139): PHPStan\Analyser\FileAnalyser->analyseFile()                                                              
     #16 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/evenement/evenement/src/EventEmitterTrait.php(111): PHPStan\Command\WorkerCommand::PHPStan\Command\{closure}()                         
     #17 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/clue/ndjson-react/src/Decoder.php(117): _PHPStan_b7fe9900d\Evenement\EventEmitter->emit()                                              
     #18 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/evenement/evenement/src/EventEmitterTrait.php(111): _PHPStan_b7fe9900d\Clue\React\NDJson\Decoder->handleData()                         
     #19 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/react/stream/src/Util.php(62): _PHPStan_b7fe9900d\Evenement\EventEmitter->emit()                                                       
     #20 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/evenement/evenement/src/EventEmitterTrait.php(111): _PHPStan_b7fe9900d\React\Stream\Util::_PHPStan_b7fe9900d\React\Stream\{closure}()  
     #21 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/react/stream/src/DuplexResourceStream.php(168): _PHPStan_b7fe9900d\Evenement\EventEmitter->emit()                                      
     #22 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/react/event-loop/src/StreamSelectLoop.php(201): _PHPStan_b7fe9900d\React\Stream\DuplexResourceStream->handleData()                     
     #23 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/react/event-loop/src/StreamSelectLoop.php(173): _PHPStan_b7fe9900d\React\EventLoop\StreamSelectLoop->waitForStreamActivity()           
     #24 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/src/Command/WorkerCommand.php(99): _PHPStan_b7fe9900d\React\EventLoop\StreamSelectLoop->run()                                                 
     #25 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/symfony/console/Command/Command.php(259): PHPStan\Command\WorkerCommand->execute()                                                     
     #26 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/symfony/console/Application.php(870): _PHPStan_b7fe9900d\Symfony\Component\Console\Command\Command->run()                              
     #27 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/symfony/console/Application.php(261): _PHPStan_b7fe9900d\Symfony\Component\Console\Application->doRunCommand()                         
     #28 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/vendor/symfony/console/Application.php(157): _PHPStan_b7fe9900d\Symfony\Component\Console\Application->doRun()                                
     #29 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/bin/phpstan(124): _PHPStan_b7fe9900d\Symfony\Component\Console\Application->run()                                                             
     #30 phar:///Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan.phar/bin/phpstan(125): _PHPStan_b7fe9900d\{closure}()                                                                                              
     #31 /Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan/phpstan(8): require('...')                                                                                                                                        
     #32 /Users/vishal.khode/drupal11/scheduler/vendor/bin/phpstan(119): include('...')                                                                                                                                                  
     #33 {main}                                                                                                                                                                                                                          
 -- ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ 


                                                                                                                        
 [ERROR] Found 1 error                                                                                                  
                                                                                                                        

PHP runtime version: 8.3.6
PHP version for analysis: 8.3.6 (from runtime)

PHPStan version: 1.11.10
PHPStan running from:
/Users/vishal.khode/drupal11/scheduler/vendor/phpstan/phpstan

Extension installer:
composer/composer: 2.7.7
composer/pcre: 3.2.0
mglaman/phpstan-drupal: 1.2.12
phpstan/phpstan-deprecation-rules: 1.2.0
phpstan/phpstan-phpunit: 1.4.0

Discovered Composer project root:
/Users/vishal.khode/drupal11/scheduler

Parallel processing scheduler:
# of detected CPU cores:   12
# of analysed files:       1
# of jobs:                 1
# of spawned processes:    1

⚠️  Result is incomplete because of severe errors. ⚠️
   Fix these errors first and then re-run PHPStan
   to get all reported errors.

Elapsed time: 7 seconds
Used memory: 128.5 MB

🇮🇳India vishalkhode

Please create MR for the same. I'll review and merge as it's very small fix. Hence, moving it back.

🇮🇳India vishalkhode

Thanks everyone for working on this. Looks good to me, Hence, merged.

🇮🇳India vishalkhode

I ran the ESlint in local and it showed errors in config_update_ui.settings.yml file. I think we should handle & fix that in a separate ticket. So, I'm Ok, if we've skipped the ESlint job here, we can enable and fix those separately. Thanks.

🇮🇳India vishalkhode

Requested some changes, hence moving it back to Needs Work. Also, let's not skip eslint and fix that as well. If that's a major change and required more changes, we can skip eslint and cslint for now and handle that in a separate ticket and limit this ticket for PHPCS and PHPStan fixes only.

🇮🇳India vishalkhode

The failing PHPUnit test on Drupal Core 9.5.x was due to the toUrl() method on node entity (e.g., $node->toUrl()) incorrectly including the URL path parameter, resulting in URL like /web/web/node/1 and causing a "Page not found" error. To fix this issue, the URL was updated from $node->toUrl() to 'node/'.$node->id(). Hence, merged.

🇮🇳India vishalkhode

The password policy status message displays as Pass or Fail when a user adds or updates their password, depending on whether it meets the configured policy constraints. However, this message was not being updated correctly when the constraints were met or failed because the replace method is no longer available in Drupal 11. The fix provided by @maursilveira addresses this issue, and I’ve also added test coverage for the same.

🇮🇳India vishalkhode

@jonathan1055 Can you please confirm, what's pending for Drupal 11 compatibility fixes ? Everything looks good now. When this is planned for merge & release ?

🇮🇳India vishalkhode

Reviewed changes and looks good to me. As I can separate tickets have been created to fix PHPCS and PHPStan issues, hence marking RTBC.

🇮🇳India vishalkhode

Thanks @maursilveira for reporting issue and MR. Reviewed changes and looks good to me. However, quite interesting how this wasn't caught by the Upgrade Status module. I'll validate and merge, if all looks good.

🇮🇳India vishalkhode

Thanks @pooja_sharma, changes looks good to me. Verified and it's seems to be working fine. However, I also noticed that we don't have test coverage which checks for Password Policy history constraint, but that's fine, we can create a separate ticket for it and fix it there. Hence, merged.
Thanks everyone for working on this.

🇮🇳India vishalkhode

@pooja_sharma: Thanks for the adding test coverage, let's not add tests in PasswordResetBehaviorsTest as that test is specifically to validating password reset functionality. And here we want verify password_history functionality, so that it doesn't accept repeated password. Lets see if we can add test coverage for following:

  • Password History constraint works as expected.
  • When user try to provide the same password again from UI, it throws an error.

For this, we can either update the existing tests i.e PasswordPolicyInterfaceTest.php or we can add a new tests in password_history submodule similar to PasswordCharacterOperationsTest.php

🇮🇳India vishalkhode

Reviewed changes and looks good to me. Hence, RTBC.

🇮🇳India vishalkhode

@VladimirAus I don't think we should fix PHPCS or PHPStan error as part of this ticket. We should limit this ticket to fix only Drupal 11 deprecation errors/warnings and failing PHPUnit tests.

Thanks for commits. 🥳
Need to make sure phpstan (next major) is green.

🇮🇳India vishalkhode

Moving this back, as there are lots of fixes needs to done on AssetVersionResolver class.

🇮🇳India vishalkhode

Hi @Finn Lewis,

I'm also not able to reproduce the issue. Can you re-validate again and also verify if you've selected Roles when configuring the Password policy and the user who's changing the password has the same role ?

🇮🇳India vishalkhode

Changes looks good to me. However, If we add test coverage for the same, that would be great.

🇮🇳India vishalkhode

Looks good to me. Hence, merged.

🇮🇳India vishalkhode

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

🇮🇳India vishalkhode

Requested some changes. Hence, moving it back.

🇮🇳India vishalkhode

Reviewed changes and looks good to me.
@vipin.mittal18 The warning you mentioned above was not due to changes made on this ticket and have been addressed separately as part of 🐛 [4.0.1 regression] password_length constraint should check plugin id in validateConfigurationForm() Fixed . Hence, merged.

🇮🇳India vishalkhode

Thanks everyone for working on this. Added test coverage for this bug and for the validations that were added as part of 📌 Password Character Length Policy allows duplicate and invalid Fixed .
@smori1983 If would be great, if you can create a separate issue for problem #2 you mentioned above with detailed steps to reproduce, so we can take a look and fix that up. Thanks.

🇮🇳India vishalkhode

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

🇮🇳India vishalkhode

Reviewed changes, looks good. However, we should also add a functional test case for the same.

🇮🇳India vishalkhode

I'm able to reproduce the issue, but clearing cache fixed the issue. The error came because arguments in password_policy_event_subscriber enty_subscriber service was updated. In release 4.0.0, the 3rd argument was service @request_stack which is now updated to @?masquerade. The service objects are cached by Drupal, so whenever there's change in service definition, the cache clearing is required. Hence, closing this ticket. Feel free to re-open the ticket, if you think it's otherwise or have any other questions/concerns.
Thanks.

Production build 0.71.5 2024