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

Fix ExcelProvider #319

Merged
merged 4 commits into from
Aug 17, 2021
Merged

Conversation

dpfaffenbauer
Copy link
Collaborator

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #318

@paulverdu
Copy link
Contributor

This fixes the problem with the processor callback function.
Only problem now that when the processor returns null, in case of the first 'headers row'. The first iteration of $dataset in Importer:runImport() runs importRow() with variable $row being null.

The parameter $data in importRow() is not nullable at the moment. This exception is catched, but is not ideal.
Making $data nullable will make $object in Importer null but this will count as 1 row being processed which is also not ideal. There for I think we should do a continue in Importer:runImport() just before the try catch:

if ($row === null) {
    continue;
}

This will not make the first $row being null count as a row being processed.

What do you think @dpfaffenbauer ?

@dpfaffenbauer
Copy link
Collaborator Author

I would actually have to test that use-case myself. I'll comeback to you with a solution

@paulverdu
Copy link
Contributor

Screen Shot 2021-08-16 at 13 38 06

So this would be my solution for this problem.

@paulverdu
Copy link
Contributor

I would actually have to test that use-case myself. I'll comeback to you with a solution

Ok, thanks

@paulverdu
Copy link
Contributor

if (null === $data) {
    return null;
}

This code in Importer:importRow() can be removed then as well since parameter $data is not nullable, so this code already never gets executed atm.

@dpfaffenbauer
Copy link
Collaborator Author

@paulverdu Did the changes you recommended. Seems that in fact you are right here :)

@paulverdu
Copy link
Contributor

@paulverdu Did the changes you recommended. Seems that in fact you are right here :)

Awesome. Hope we can merge this soon, so we can continue testing our application 😄

@dpfaffenbauer dpfaffenbauer merged commit 8df9ec6 into instride-ch:master Aug 17, 2021
@dpfaffenbauer
Copy link
Collaborator Author

done :)

@dpfaffenbauer dpfaffenbauer deleted the issue/318 branch August 17, 2021 09:05
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