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

Remove StatefulInterface from type hint #39

Merged
merged 1 commit into from
May 14, 2014

Conversation

winzou
Copy link
Contributor

@winzou winzou commented May 6, 2014

To be fully compliant with the new property access strategy.
Otherwise, I can't use the TwigExtension for example.

{
return $this->getStateMachine($object, $graph)->getCurrentState()->getName();
}

/**
* @param StatefulInterface $object
* @param string $graph
* @param object $object
Copy link

Choose a reason for hiding this comment

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

should this rather be a common interface? is there any other way to accomplish this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know there is no other way. We can dynamically define under which property path is the state of the object, so it can't be achieved by a static interface.

Copy link

Choose a reason for hiding this comment

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

there must be a way, cc/ @matthiasnoback
I feel like removing interfaces like this should be thought well

@winzou
Copy link
Contributor Author

winzou commented May 6, 2014

@yohang @cordoval if the passed object is not the good one, then the Factory won't find an associated loader. But as per https://github.com/yohang/Finite/blob/master/src/Finite/Factory/AbstractFactory.php#L33 this won't throw any exception. Maybe we should throw one if $loader is null there?

@yohang
Copy link
Owner

yohang commented May 6, 2014

There is 2 problems here,

  1. The interface is no longer useful, and we have no way to "flag" an object as Stateful. We should maybe process checks on the available properties defined by the state accessor.

    The StatefulInterface is still interesting for people that define only one graph, with minimal configuration (with the default property_path being finiteState)

  2. As @winzou says, we have to identify incompatible objects given to the factory, but that's not easy as the loader isn't mandatory. I don't see use case for using the Factory without Loader, but removing this won't be BC.

In all case, I'll merge this PR as there is no more StatefullInterface typehints in Finite code, as this is wrong with the property path strategy, so there is no reasons to keep it in Context and Twig extension.

To be fully compliant with the new property access strategy
@winzou
Copy link
Contributor Author

winzou commented May 14, 2014

ping @yohang

yohang added a commit that referenced this pull request May 14, 2014
Remove StatefulInterface from type hint
@yohang yohang merged commit 4345cef into yohang:master May 14, 2014
@yohang
Copy link
Owner

yohang commented May 14, 2014

Missed this one, sorry.

Merged ;)

@winzou winzou deleted the fix/interface branch May 14, 2014 08:38
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.

3 participants