php-menu
php-menu copied to clipboard
Fix tests and also add phpdoc tags / submenu method completion
fixes #7
I realize submitting a PR for each item would be better, but they shouldn't break any functionality.
[x] removeItem should return true and be optimal for runtime [x] phpunit fix for newer versions [x] some phpdoc fixes for IDEs [x] magic method completion for submenus [x] fix expected output when running on windows or non-posix osx (\r\n and \r respectively)
HHVM fails, but shouldn't composer have "phpunit/phpunit:*"?
Also, I don't see any use of mockery in your tests at the moment...
Hey, looks good to me with a few exceptions.
The project was initially set up using tabs for indentation. Could you either stick with this, or submit a separate PR just for converting to spaces (PSR-2), then rebase this PR on top of that? For what it's worth, I think just converting to tabs will save you the headache of a difficult rebase.
It seems like it's possible for there to be two items that should be removed in the removeItem code, I think that's the purpose of not returning early - could you prove me wrong on that?
- Totally forgot tabs :) Also, didn't see any editorconfig (http://editorconfig.org/)
- item id's are supposed to be unique, so that's why I added return statements. But it seems I used the wrong phpdoc return..
Yeah I never got around to editorconfig or anything like that. My editors adapt to the project style automatically so I more or less assume that others do the same.
Technically this is possible:
$item = $collection->makeItem('Example', '/example');
$collection->addItemInstance($item, 10);
$collection->addItemInstance($item, 10);
In this case, removeItem would only remove one of the items. Not sure if it should count as undefined behaviour that's OK to break, though.
Also, there is this:
$item = $collection->makeItem('Example', '/example');
$collection->addItemInstance($item, 10);
$item2 = $collection->makeItem('Example', '/example');
$collection->addItemInstance($item2, 10);
$collection->removeItem('example');
// $item2 is deleted now, but is $item1 also deleted?
Adding $item2 should actually throw an exception if you ask me, since it's already in the $items property. Or maybe it would make more sense if spl_object_hash is used for the id hash?
Maybe this particular change should be put in a separate pull request. I think it's a somewhat difficult question and a breaking change, and I would like to merge your other changes.
Ok, disregard this PR. I will redo the changes and submit a new PR.