Reviewed changes, looks good to me. We can remove temp commit and request for merge. Hence, marking RTBC.
baluertl → credited vishalkhode → .
Reviewed changes, looks good to me. Hence, RTBC.
Reviewed changes, looks good to me. Hence, RTBC.
Reviewed changes, looks good to me now. Hence, RTBC.
Reviewed MR !49, looks good to me now. Hence, RTBC.
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited 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.
Closing as it's duplicate of 📌 Automated Drupal 11 compatibility fixes for acquia_cms_starter Needs review .
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.
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
rajeshreeputra → credited vishalkhode → .
vishalkhode → created an issue.
Merged. Hence, closing. Will release shortly. Thanks.
Thanks @Rajeshreeputra for working on this. The Config Filter → module dependency has been removed as it's not required.
vishalkhode → made their first commit to this issue’s fork.
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.
Fixed and merged.
Thanks @nord102. Fixed and release as part of 3.3.11 → .
@ok4p1: Yes, we've release by this week.
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.
Reviewed changes, looks good. Hence, RTBC.
Reviewed changes, looks good to me now. Hence, RTBC.
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.
vishalkhode → changed the visibility of the branch 3471672-the-module-fails to active.
vishalkhode → changed the visibility of the branch 3471672-the-module-fails to hidden.
vishalkhode → created an issue.
@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.
Overall changes looks good to me. One minor change requested, please take a look. Hence, moving it back.
klausi → credited vishalkhode → .
Reviewed changes, looks good to me. Hence, RTBC.
vishalkhode → made their first commit to this issue’s fork.
Reviewed changes, looks good to me. Hence, RTBC.
Making dataProviders to static in PHPUnit tests is must requirement for D11 compatibility. Hence, this needs to fixed first.
The Scheduler → module has now 2.x-dev → dev release available, so we can start working on this. Hence, re-opening this.
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.
@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...
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.
Looks good to me. Hence, merged.
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
Please create MR for the same. I'll review and merge as it's very small fix. Hence, moving it back.
Thanks everyone for working on this. Looks good to me, Hence, merged.
Closing as mistakenly created twice 📌 Replace dynamic extension.list.$type service with extension.path.resolver service object Active .
vishalkhode → created an issue.
vishalkhode → created an issue.
Closing as it's covered in 📌 Fix validate pipeline Needs review .
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.
vishalkhode → made their first commit to this issue’s fork.
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.
Closing as this is fixed in 📌 Fix Validate pipeline Fixed .
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.
Closing, as this is merged now.
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.
vishalkhode → made their first commit to this issue’s fork.
vishalkhode → changed the visibility of the branch 4.0.x to hidden.
@jonathan1055 Can you please confirm, what's pending for Drupal 11 compatibility fixes ? Everything looks good now. When this is planned for merge & release ?
Reviewed changes and looks good to me. As I can separate tickets have been created to fix PHPCS and PHPStan issues, hence marking RTBC.
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.
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.
Reviewed changes, looks good to me. Hence, merged.
vishalkhode → made their first commit to this issue’s fork.
@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
Reviewed changes and looks good to me. Hence, RTBC.
@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.
Moving this back, as there are lots of fixes needs to done on AssetVersionResolver
class.
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 ?
Changes looks good to me. However, If we add test coverage for the same, that would be great.
Closing as this is duplicate of 🐛 Fatal error when changing password when password_policy_history is enabled RTBC .
vishalkhode → changed the visibility of the branch project-update-bot-only to hidden.
Looks good to me. Hence, merged.
vishalkhode → made their first commit to this issue’s fork.
vishalkhode → made their first commit to this issue’s fork.
Requested some changes. Hence, moving it back.
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.
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.
Looks good to me.
vishalkhode → made their first commit to this issue’s fork.
Thanks everyone for the changes. Looks good to me. Hence, moving it ahead.
vishalkhode → made their first commit to this issue’s fork.
Reviewed changes, looks good. However, we should also add a functional test case for the same.
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.