wieni / wmcontroller

Adds support for bundle-specific controllers for Drupal 8 entities.
MIT License
3 stars 4 forks source link

Presenters: init #29

Closed frizinak closed 7 years ago

frizinak commented 7 years ago

See readme for usage

Almost no testing has been done, so don't merge yet.

oh... any thoughts?

frizinak commented 7 years ago

what's up with the hard to maintain AbstractPresenter (implementation of the huge EntityInterface)

- @RobinHoutevelts irl 2017

Perhaps instead of piggy backing on EntityPresentedEvent we also dispatch a SomethingPresentedEvent with a setPresenter method whos argument only needs to implement setEntity and getEntity, AbstractPresenter then only provides the protected $entity property.

class SomethingPresentedEvent
{
    public function __construct (HasPresenter $something);

    public function getSomething();

    // Where PresenterInterface no longer extends EntityInterface
    public function setPresenter(PresenterInterface $presenter);
}

Pros:

Cons:

But let's be real, is that con valid? If we're implementing custom classes with presenters, why would we assume id() / toUrl() in twig?

Interfaces have no meaning in twig anyways.

So yeah, might as well change this unless someone comes up with a real 'con' ;)

Ideas to rename 'Something' to well... something more meaningful?

spoit commented 7 years ago

OMG I love presenters

frizinak commented 7 years ago

removed EntityInterface contract

seems backward compatible for what it's worth

RobinHoutevelts commented 7 years ago

Ideas to rename 'Something' to well... something more meaningful?

Entity isn't that bad a name. I just checked at Laracasts/Presenter Presenter.php file and even there entity is being used. I'd expected model :)

Glad the dependency on EntityInterface is removed for presenters. 👍

frizinak commented 7 years ago

well I went with 'PresentedEvent' and the internal name is $item

I'd say merge if we have no issues until tuesday eve, close to impossible to break non-presenter sites anyways.

ps: since an event is dispatched for every single @included var, we might see some performance issues. If this is the case we should only dispatch for HasPresenter implementors. Doubt it though