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: add recordMany function #272

Closed
wants to merge 2 commits into from

Conversation

mauri870
Copy link

@mauri870 mauri870 commented Jun 9, 2022

No description provided.

@cyrildewit
Copy link
Owner

What would be a use case for this?

@mauri870
Copy link
Author

mauri870 commented Jun 11, 2022

@cyrildewit One use case is when migrating from a different solution to yours, for example from a integer column views count to the eloquent-viewable package. We need to seed the views count we already have and using views($viewable)->record() does an insert per view so it's extremely slow, where as recordMany insert multiple rows with a single insert query.

@cyrildewit
Copy link
Owner

Hi, a bit late, but I will postpone this idea for now and provide a clarification on the why.

For one-time data migrations like this, I wouldn’t recommend using the views() helper. Instead, working directly with the View Eloquent model gives more control over performance and lets us manage specific details, like custom timestamps and visitor IDs, more explicitly.

I prefer keeping the interface simple and transparent, as this avoids adding “magical” options that might mislead developers. With recordMany(), for example, it can seem like a one-size-fits-all solution, but it’s slightly ambiguous: it auto-fills the visitor and viewed_at columns with a new visitor ID and the current timestamp. For migrations needing historical data, this isn’t ideal.

I appreciate the suggestion!

@cyrildewit cyrildewit closed this Oct 28, 2024
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.

2 participants