Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

[RFC] PhpcrProvider refactoring #257

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dantleech
Copy link
Member

@dantleech dantleech commented May 22, 2016

Fixes #255
Based on #256

WIP.

This PR refactors the PhpcrMenuProvider to throw errors more often.

The current problem is that has was hiding all of the Exceptions which were happening in the find method and interpreting then as "menu not found".

I think the only time it should return false is when the menu document was not found. If the menu was found and it subsequently encountered another problem, then we should throw an exception.

F.e. if the menu document does not implement ItemInterface, then what is it doing at the menu basepath ? The most likely scenario is that the developer forgot to add the ItemInterface to the document.

It does alot of reorganizing, making the code easier to understand (I hope!).

$document = $this->find($name, $options);

if (null === $document) {
return false;
Copy link
Member Author

Choose a reason for hiding this comment

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

Now .. the only situation where has returns false is when the document does not exist. If the document is not a ItemInterface instance an exception will be thrown.

@dbu
Copy link
Member

dbu commented May 23, 2016

great job, so much better!

i guess we should add a note to the changelog to explain, in case somebody was working around strange data in their repository and now getting the clearer exceptions and being confused by them.

@dbu
Copy link
Member

dbu commented May 23, 2016

can you rebase on master so we see what really changes in the test? i just merged #256

@dantleech
Copy link
Member Author

Rebased, but is likely now taking the wrong approach as we are discussing in #254

@wouterj wouterj modified the milestone: 2.1 Jun 18, 2016
@ElectricMaxxx ElectricMaxxx modified the milestones: 2.2, 2.1 Nov 28, 2016
@ElectricMaxxx
Copy link
Member

This one and its issue is tagged with milesonte for version 2.2. Do we need that?

@ElectricMaxxx
Copy link
Member

Anything going on here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants