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

Add asset indexing to Craft cli. #3595

Merged

Conversation

ryanpcmcquen
Copy link

Closes #2980.

@ryanpcmcquen
Copy link
Author

ryanpcmcquen commented Jan 8, 2019

I actually think the default should be false (0) on $cacheImages here, but since the GUI default is true, I made the default true here as well. I'm guessing a lot of people don't want to fill their database (host volume) with an image they are already storing somewhere else (like on an S3 bucket).

But, that discussion/decision shouldn't be tied to this PR anyway.

@brandonkelly
Copy link
Member

They get stored in the storage/ folder, not your database, and it’s a prerequisite to generating new image transforms, so a reasonable argument could be made either way.

@andris-sevcenko
Copy link
Contributor

andris-sevcenko commented Jan 16, 2019

Sorry for the delayed response - thinks have been a tad hectic lately.

Can you, please, reuse the same indexing session id for one indexing session instead of generating a new one for every file that is getting indexed?

@ryanpcmcquen
Copy link
Author

@andris-sevcenko, thank you for reviewing this! How would you recommend doing that? Can it just use a timestamp?

@andris-sevcenko
Copy link
Contributor

@ryanpcmcquen you should call the Craft::$app->getAssetIndexer()->getIndexingSessionId() method once per session and use the returned value for all actual indexing calls.

Also, the docblock above the class definition does not look like anything that Craft has. (starting here)

@ryanpcmcquen
Copy link
Author

@andris-sevcenko, isn't this similar?
https://github.com/craftcms/cms/blob/3.1/src/console/controllers/MigrateController.php#L24

I changed it to use the session id as requested. 🎉

@andris-sevcenko
Copy link
Contributor

@ryanpcmcquen You are correct. It is similar. I guess there was a precedent, after all but I failed to find it :)

@ryanpcmcquen
Copy link
Author

@andris-sevcenko, I will restore it then! Anything else?

@ryanpcmcquen
Copy link
Author

@andris-sevcenko, I restored the docblock, any other comments on the PR?

@andris-sevcenko
Copy link
Contributor

@ryanpcmcquen ah sorry, totally forgot about this one today - it was on my list, too :/

I'll take care of this tomorrow my time, sorry for the delay!

@brandonkelly brandonkelly changed the base branch from 3.1 to develop January 17, 2019 18:38
@andris-sevcenko andris-sevcenko merged commit d54bbd5 into craftcms:develop Jan 18, 2019
@andris-sevcenko
Copy link
Contributor

Thank you for your patience, Ryan. You have been more than awesome!

@ryanpcmcquen
Copy link
Author

Thank you Andris!

brandonkelly added a commit that referenced this pull request Jan 18, 2019
@khalwat
Copy link
Contributor

khalwat commented Jan 19, 2019

Wow, thank you @ryanpcmcquen -- saved me from writing it!

@ryanpcmcquen
Copy link
Author

@brandonkelly, should I have set the @author in the docblock? Wasn't sure what the policy was here ...

@brandonkelly
Copy link
Member

brandonkelly commented Jan 22, 2019

@ryanpcmcquen Ah yeah I overlooked that too. Added for the next release.

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

4 participants