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

Event "data_definitions.import.total" never gets dispatched #320

Merged
merged 7 commits into from
Aug 18, 2021

Conversation

paulverdu
Copy link
Contributor

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? yes
Branch? master

In Importer.php the event "data_definitions.import.total" never gets get dispatched when $data is an object of type \Wvision\Bundle\DataDefinitionsBundle\Provider\TraversableImportDataSet.

Fixed this by adding Countable interface and setting the count on object creation.

Paul Verdu added 5 commits August 16, 2021 15:08
By counting the traversable object before creating the IteratorIterator object, no exception will be thrown when looping over the dataset after counting the elements.
@dpfaffenbauer
Copy link
Collaborator

works for me, what does @dkarlovi think?

Copy link
Collaborator

@dkarlovi dkarlovi left a comment

Choose a reason for hiding this comment

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

Small tweaks, otherwise good with me.

{
$this->iterator = new \IteratorIterator($iterator);
$this->count = iterator_count($iterator);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this lazy? Only count it if the count is requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible to do a count on a iterator since you can only rewind ones. That's why im doing this in the constructor before the IteratorIterator object is created. The count is also always requested in the importer.

@dkarlovi
Copy link
Collaborator

@dpfaffenbauer looked at it, posted a review.

@paulverdu
Copy link
Contributor Author

@dpfaffenbauer If we could merge this one as well. Would be awesome 😄

@dpfaffenbauer dpfaffenbauer merged commit 76b4c8a into instride-ch:master Aug 18, 2021
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.

3 participants