🇮🇳India @vishalkhode

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

Not able to reproduce issue as well. Verified that getMapping() method is called multiple times, but, due to the condition i.e if (!$this->mapping) {, it only executes once. Please add steps to reproduce and re-open, if we still see this issue. Hence, closing.

Reviewed changes, overall looks good to me. A PHUnit test covering above scenario would be good or we should handle separately ? As we can also then remove the $reset parameter from getKeyValue() and getKeyValues() methods as cache is now being updated / deleted in CRUD operations and ensuring it always returns the correct value. Hence, RTBC.

Requested some changes in the MR. One question here, should this provider function similar like the Environment Key provider, meaning should only read values from the state rather than saving, reading and deleting value as well from the state ?

Reviewed changes, looks good to me. Hence, RTBC.
Note: Core Version requirement has been updated and Drupal Core 8 support is dropped. Probably this requires a new minor release.

vishalkhode → created an issue.

In !MR 6 Looks like there's a complete new feature has been introduced i.e Provide support for external file url for key. I'm not sure if this should be supported or not. In any case, this should be implemented and discussed separately. We should limit this ticket to fix the bug only.

I've to verified and not seeing any error with 1.1.x. Looks like this is already fixed & merged as part of 🐛 TypeError on EmbedCodeUrlBuilder on new revision update for an existing asset/image on Acquia DAM Active . Hence, closing.

To fix the issue, I think we can do the following:

1. Update the method to also update the static cache. For example:
   

     $key_values = &drupal_static('getKeyValue', []);
     $key_values[$this->id()] = $key_value;
   

2. Update the method to remove the value from the static cache as well. For ex:
   

     $key_values = &drupal_static('getKeyValue');
     unset($key_values[$this->id()]);
   

3. Lastly, we should consider removing the $reset argument from the method and the related code, since the static cache should now always reflect the correct and updated value. Are there any edge cases where we might still need the flag ?
4. Add / Update the PHPUnit tests to cover all above scenarios.

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

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

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

rajeshreeputra → credited vishalkhode → .

ankitv18 → credited vishalkhode → .

vishalkhode → created an issue.

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

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

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

Thanks @rajeshreeputra for working on this. Changes looks good. Hence, merged.

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

Thanks @vipin.mittal18 & @Rajeshreeputra for reporting issues. The CI was failing earlier, I've updated the CI template and rebase the MR, though there are couple of allowed failure CI jobs and some of them are failing, we can create a separate ticket to fix this.

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

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

vishalkhode → created an issue.

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

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

hestenet → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

ankitv18 → credited vishalkhode → .

ankitv18 → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

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

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

ankitv18 → credited vishalkhode → .

ankitv18 → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

rajeshreeputra → credited vishalkhode → .

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.

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

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.