Rewrote passName into $pass as requested #50
Conversation
Also tried to write some unit tests, though my phpunit was playing up, not sure why, hopefully it tests successfully on here…
|
Hi Sasha, Thank you for including the tests. I see that the pass name is still being set through the constructor. I think this is problematic in scenarios where the PassFactory is packaging multiple passes. Since the pass name is always the same value, each pass would overwrite the previous one. This could be solved by including the passName as a parameter of the package() method so that a different name can be passed in each time. |
Datetime is now defined in the tests file as the tests fail if date.timezone is not set on the php server.
|
@g Sorry about that, didn't really think about it as I'm only ever packaging single passes. Have moved it into the package() function. |
| @@ -9,6 +9,6 @@ | |||
| * file that was distributed with this source code. | |||
| */ | |||
|
|
|||
| $loader = require_once __DIR__ . "/../vendor/autoload.php"; | |||
| $loader = require __DIR__ . "/../vendor/autoload.php"; | |||
There was a problem hiding this comment.
What is the reason for this change? I'd expect the autoload file to only be loaded once.
There was a problem hiding this comment.
The test failed when set to require once. I can't remember the exact error or the explanation for it, but this fixed it. Plus it's only in the tests that this has been changed, so not too much to worry about... I can try and reproduce the error if you want, but I am certain that it wasn't something that I later discovered that was wrong with my environment...
There was a problem hiding this comment.
Ok. I've taken this out of this pull request as I'm not convinced that is correct. We can address it separately if it continues to be problematic.
|
Yeah I'm not sure about the require in the test, but I know require_once wasn't working for me. Assume it was just something on my setup. |
Also tried to write some unit tests, though my phpunit was playing up,
not sure why, hopefully it tests successfully on here…