Pune
Account created on 21 March 2016, about 9 years ago
#

Merge Requests

More

Recent comments

🇮🇳India rajeshreeputra Pune

Hello @byrond,

You are correct — version_id and external_id are not required during the migration process. After migration, the cron in the Acquia DAM module should handle updating version_id and external_id regularly and automatically.

Thank you for bringing this up!

🇮🇳India rajeshreeputra Pune

rajeshreeputra changed the visibility of the branch 3527397-allow-subcategories-bulk-import to hidden.

🇮🇳India rajeshreeputra Pune

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

🇮🇳India rajeshreeputra Pune

Updated MR with database operations instead of media entity resave, resulting in reduced CPU overhead and improved memory efficiency. This change has led to faster execution times, which should significantly enhance the migration process.

Please review the changes and share your feedback.

🇮🇳India rajeshreeputra Pune

Fixed, created MR, requesting review.

🇮🇳India rajeshreeputra Pune

The code is needed, if but need to move below the queue create item call.
If the code is removed then the assets will always show the media item status as outdated.

🇮🇳India rajeshreeputra Pune

Most probably issue introduced because of the AvailabilityPublishingStatusCompare.php.

The AvailabilityPublishingStatusCompare field is calling $this->assetUpdateChecker->checkAssets($values->_entity) for every single row in the media library view during the render() method.

The Problem

N+1 Performance Issue:

  • Media library loads: 12 assets per page
  • Each asset triggers: checkAssets() call in render()
  • Result: 12 individual API calls to Acquia DAM

Why this happens

  • The render() method is called for every row when the view is displayed.

Solution

  • Remove the "Availability/Publishing status" field from Media library.
🇮🇳India rajeshreeputra Pune

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

🇮🇳India rajeshreeputra Pune

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

🇮🇳India rajeshreeputra Pune

@rlhawk this need to cherry pick into 4.0.x branch as well.

🇮🇳India rajeshreeputra Pune

Major Performance Improvements

  • Chunked Processing: Instead of loading all media items at once, the code now processes them in smaller batches of 100 items.
  • Proper Batch API Usage: The batch operations are now structured to handle each chunk separately, preventing memory exhaustion and timeouts.
  • Improved Progress Tracking: Added detailed progress tracking using Drupal's state API, making the process more robust and providing better user feedback.
  • Optimized Resource Usage: Service instances like time and current user are now retrieved once at the beginning of each batch operation rather than for each media item.

Implementation Details

  • Added new batch operation methods (initializeMediaBatch, batchUpdateMediaItems, finalizeMediaBatch) to handle different phases of the migration process.
  • Implemented paged database queries to fetch only the media items needed for each batch operation.
  • Added proper progress tracking that persists between batch operations.
  • Maintained all the existing functionality while making the process much more efficient.

Expected Results
With these changes, the migration should:

  • Complete without timing out.
  • Use significantly less memory.
  • Provide better progress information to users.
  • Likely reduce the total processing time.
🇮🇳India rajeshreeputra Pune

Removed #3483550 from list as it depends on #3306506 & #3524922

🇮🇳India rajeshreeputra Pune

Keeping this intact instead created separate issue for dropping support of Drupal core below 10.3 version.
💬 Drop support of Drupal core below 10.3 version. Active

🇮🇳India rajeshreeputra Pune

Update ticket to Drop support of Drupal core version below 10.3.

🇮🇳India rajeshreeputra Pune

Removed 📌 Modernize the Drush commands Needs review from list as it depends on 📌 Remove support for Drupal < 9.1 Needs review .

🇮🇳India rajeshreeputra Pune

@ptmkenny, we should update the title, description and MR accordingly to support Drupal core >=10.3 version in 4.x version.

🇮🇳India rajeshreeputra Pune

Hello @ptmkenny,

Why is the list an ordered list? Are the issues dependent on one another? (I'm guessing no because some of the later issues have already been committed.)

The issues are not dependent on one another, and it is not necessary for the list to be ordered. We can remove the serial number column from the table.

The big question is should Key drop support for Drupal 8 and/or 9.

The decision to drop this support has already been considered, and it will be removed in the next major version. For more details, please refer to the discussions in the key channel here.

If support for Drupal 8/9 is dropped, I think we should not just "require Drupal 10" but actually take the opportunity to modernize the code and use some of the new features of Drupal 10.

I agree that we should not only require Drupal 10 but also take the opportunity to modernize the code and utilize some of the new features available in Drupal 10. This will be taken into account as part of the next major release, but I will defer to @rlhawk regarding the decision to drop support for Drupal 8 and/or 9 in the 1.x or 2.x.

🇮🇳India rajeshreeputra Pune

Need to resolve the conflits.

🇮🇳India rajeshreeputra Pune

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

🇮🇳India rajeshreeputra Pune

Sorry for the confusion caused by previous comment.

I would like to share a use case to clarify the expected behavior regarding configuration overrides with the Key module.

For example, let's say we have the following configuration:

my_module.settings:
  key1: value1
  secret: secret_value
  • We override the secret value using the Key module.
  • When we access the configuration value in code as:
    $config = \Drupal::config('my_module.settings')->get('secret'); the returned value should be the overridden secret from the Key module, i.e., secret_value from the key configuration.
  • When we access the configuration value in CLI:
    ./vendor/bin/drush cget my_module.settings secret the returned value should be null.
  • Even when accessing via configuration export it should be null, in my_module.settings.yml file.
🇮🇳India rajeshreeputra Pune

But should $config = \Drupal::config('my_module.settings')->get('my_setting'); give me my setting value when this module is in place?

This should not give the value, as value in config now cleared by the override.

Or should I use $config = \Drupal::service('key.repository')->getKey('my_key')->getKeyValue();?

This should give the value from the key.

🇮🇳India rajeshreeputra Pune

I think we can make things a bit clearer for users by updating the label.
Right now, "Clear overridden value" might leave some people unsure about what it actually does. I suggest changing it to "Clear overridden value from the configuration item." This way, it’s more specific and helps users understand exactly what will happen when they click it.

🇮🇳India rajeshreeputra Pune

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

🇮🇳India rajeshreeputra Pune

rajeshreeputra changed the visibility of the branch 2972436-ability-to-map to hidden.

🇮🇳India rajeshreeputra Pune

Agree with @lind101; this requires significant changes and a thorough refactor.


🇮🇳India rajeshreeputra Pune

As unable to reproduce with latest dev of key and dependencies mentioned in description, will close the issue as it is functioning as designed. Please feel free to open a new issue if the problem persists.

🇮🇳India rajeshreeputra Pune

Resolved conflict and rebased.

🇮🇳India rajeshreeputra Pune

Incorporated feedback, hence setting to needs review.

🇮🇳India rajeshreeputra Pune

Based on the ongoing discussions in the referenced issues, will proceed with the exact ask of this issue without introducing new validations.

Requesting review.

🇮🇳India rajeshreeputra Pune

Agree with you that this will introduce compatibility issues and should be implemented only in a new major version.

🇮🇳India rajeshreeputra Pune

Thank you @mxr576,

This is kinda great learning while working on this issue about choosing between memory cache and cache bin.
It's all about asking the right questions on how sensitive and secure the data needs to be. We need to think, "Is this data okay to just be around for a bit to keep it safe or stored for longer?".
Memory cache is fine for quick stuff, but if you need it saved, then cache bin is there, just need to be cautious about where the data might end up.
So yeah, it’s all about figuring out what’s most important for the situation, and then choose accordingly.

🇮🇳India rajeshreeputra Pune

Added return type to all interfaces to discuss and finalise accordingly. Then will add return type to all methods in classes.

🇮🇳India rajeshreeputra Pune

Cleanup completed, requesting review.

Referencing existing issue to address PHPStan related warnings.

🇮🇳India rajeshreeputra Pune

Implemented cache bin in latest commit. Requesting review.

🇮🇳India rajeshreeputra Pune

📌 Update README.md as per README.md format Needs review and 📌 Fix eslint test Active now merged, added 📌 Modernize the Drush commands Needs review as ready for review.

🇮🇳India rajeshreeputra Pune

With the understanding how the public:// and private:// stream wrappers function with S3FS. Will close the MR!6(keeping for future reference) and create new one.

🇮🇳India rajeshreeputra Pune

Added Reference to release plan 1.21 version.

🇮🇳India rajeshreeputra Pune

Updated MR with example as stated in #2, requesting review.

🇮🇳India rajeshreeputra Pune

I have not yet utilized the s3fs module in Drupal, but I will give it a try. Then accordingly update here.

🇮🇳India rajeshreeputra Pune

rajeshreeputra changed the visibility of the branch 2985590-static-cache-does to hidden.

🇮🇳India rajeshreeputra Pune

Hello @cmlara, updated to work with realpath, stream wrapper and remote urls. Could you please validate with s3 once. Will keep this in needs work status.

Production build 0.71.5 2024