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

Data array should be properly initialized on reset #38

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

a-lutz
Copy link

@a-lutz a-lutz commented Feb 14, 2018

In Behat tests I had some nasty notices like "Notice: Undefined index: time in vendor/pomm-project/pomm-symfony-bridge/sources/lib/DatabaseDataCollector.php line 62"

Data reset should set back data array to construct values, not an empty array.

@sanpii
Copy link
Member

sanpii commented Feb 14, 2018

Thank you.

I have just a comment about design: I think it’s a good idea to create a new Data class to avoid method with side-effet. Then you can write $this->data = new Data() instead of $this->initData(). What do you think?

@a-lutz
Copy link
Author

a-lutz commented Feb 14, 2018

Hello,
Could it be used somewhere else ? It looks more like a purely internal array to me (actually just realize it's never declared)

@sanpii
Copy link
Member

sanpii commented Feb 14, 2018

Could it be used somewhere else ?

No, just here.

It looks more like a purely internal array to me

Yes, but having something more strict than an array seens a good idea.

At least, can you change the initData method to return the data instead of modify the property directly.

@a-lutz
Copy link
Author

a-lutz commented Feb 14, 2018

That I can :)

@sanpii sanpii added this to the 2.5.1 milestone Feb 14, 2018
@a-lutz
Copy link
Author

a-lutz commented Feb 15, 2018

Thank you Sanpii :)
Do you have an idea for the date of 2.5.1 ?

@sanpii
Copy link
Member

sanpii commented Feb 15, 2018

When your PR will be merged 😋 Maybe it’s also a good idea to fix #34

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