Account created on 4 February 2011, over 13 years ago
#

Merge Requests

Recent comments

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

Excited to see what you come with here. I'm not surprised there are some inconsistencies and issues with the refactoring to where queueing occurs mid-project.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

I would like to push this forward if possible still. @apotek what are your thoughts on some of the issues you raised?

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

Thank you for creating this follow-up issue.

@apotek if you have time, feel free to work on this too, just let me know so we don't duplicate effort.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

trim() requires a string, but $row->get(configuration['field']) returns a field

What is a "field"? I'm trying to work on a fix, but genuinely unsure what the code change should be to prevent the issue.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

adamzimmermann β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

I just ran into this issue and confirm that it causes a WSOD.

I can also confirm that the patch solves the issue.

I haven't done extensive regression testing, but my initial testing of our endpoints shows no issues.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

The initial MR was merged, but I would like to keep this open for adding the final polish/fixes as the work I did was more of a prototype of the functionality and not feature complete.

@apotek has noted some of those changes in his comments above.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

I think, given the option of having to re-queue 1 million items from the start, if the process is interrupted, versus possibly having some paging overlap or possible interstitial changes (which could be queued later with a date-based query), I would prefer having to re-queue and re-migrate a few hundred over having to requeue and remigrate a million. I think even a "sloppy" resume is worth it. My $0.02

When you put it that way, it makes my concern seem like less of an issue haha.

I have a MR coming. I don't think it's ready for merging or final review, but it could be a interim solution that we use via patch and continue to improve on as time permits. I'm open to feedback on the approach here.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

Thank you for creating this issue and providing such a detailed request.

I want to acknowledge that I 100% see the desire for this and the value, but I have two concerns off-hand.

  1. The complexity around managing the state of the current page/the page that should be resumed from and keeping that separate for different types of requests. This is solvable in my mind though.
  2. The issue around the result set changing between the initial request and the next request, which might completely alter the items in a given page of results depending on the sorting being used. This is potentially solvable if new items are only ever added to later pages, but that is something we need to determine based upon the sorting algorithm being used.

We will have to think about this more when we get to this.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

@pratik_kamble thank you! This just removed the need for a patch from a PR I had in progress.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

The screenshots are compelling. +++

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

I went with this approach:

or keep it as is and create a new function called retrieveContentItem() :p that we call from within getContentItem()?

I named it slightly different though.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

Currently, orange_dam drush commands specify the migration with an `--migration` option. But the migrate module specifies it as a bare argument. IOW, we do `drush orange-dam:migration-run --migration=` but the migrate module does it this way `drush migrate:import `

@apotek I actually tried to go down that road and if we go that route, we will lose the ability to not pass a migration and have the interaction choice tool. So I can see both sides of the argument. What are your thoughts?

Cannot add a required argument after an optional one.

I'm going to merge the changes we have so far, and we can continue the discussion about this.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

I'm going to take a pass at understanding the scope/feasibility of this.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

@apotek I believe the changes I made address the needs you surfaced. It should pass through any options that are sent to it.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

I have solved the issues and this is ready for review.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

Alternatively, an improvement could be to remove the `asset_formats` key all together and add it to the configuration of the content types, since that is actually what this config key is doing.

I believe this is what you showed in your comment πŸ› Schema missing for `asset_formats` key in module settings. Fixed above, and I'm liking it!

This is the easy part. The harder parts:

You are correct.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

@markdorison I think that would be a good idea. Do you want to do that?

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

Thank you for submitting this. I like the proposed solution+++

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

protected function displayMigrationQueueListing(array $tags = []): void {

@apotek what branch did you see this on? I'm wondering if you just need to use the latest code.

πŸ‡ΊπŸ‡ΈUnited States adamzimmermann

@apotek that is helpful, thank you!

Production build 0.69.0 2024