Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/lsv find and replace missing images #439

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

eddiesshop
Copy link
Collaborator

This migrator for LSV relied on a pre-populated list of URLs and relative file paths which were found in their post content. Using the filenames from the relative file paths, the migrator attempts to find the file in the local media repository (wp-content/uploads, as well as some media archives provided by LSV).

In order to shave some execution time, I created a reusable function in Attachments.php that indexes and caches the local media library. I think this might be useful going forward, so I'd like the team to review this and provided feedback.

  • confirmed that PHPCS has been run

@eddiesshop eddiesshop requested a review from a team February 20, 2024 20:09
Copy link
Member

@naxoc naxoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a comment for the changes to Attachment.php:

I like this idea a lot, but I have a couple of concerns:

  • There is no cache invalidation - we'd most certainly need that. But:
  • Even if we did a time based cache invalidation – we wouldn't really be able to use the cache on commands that would be writing to the file system because the cache would always be "a step" behind. We'd have to implement some hook to invalidate the cache on write and that would likely put us right back where we were unless we can figure out a sneaky way to "add to the cache" instead of re-generating.
  • While I agree that the slowness can suck the life out of most people, I think it's best to just use the slow way – at least on the "final" run of a command. We could maybe still use this for local development where precision is less important.

To sum up: caches always make me scared and suspicious and I'm not sure we can keep up with the truth for the file system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants