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

3.0 RFC: Reintroduce 'AppModel' / Related: Renaming 'Post.php' to 'PostEntity.php' (consistency across MVC domains) #4421

Closed
ionas opened this issue Aug 28, 2014 · 16 comments
Milestone

Comments

@ionas
Copy link
Contributor

ionas commented Aug 28, 2014

Reintroduce 2.x AppModel.php in form of 3.x AppTable.php and AppEntity.php.
Any Table / Entity extends AppTable / AppEntity instead of Core's Table / Entity.

Why?

  1. It is close to what CakePHP 2.x does
  2. It saves newbies time to figure out things, helps 3.0 adoption.
  3. Simple way to add and configure Behaviors for all Tables/Entites (for instance add TimestampBehavior to all Tables, to mimick 2.x which I personally love.)
  4. Simple opt out (don't call parent::initialize())

Cons:

  1. Distracts newbies/switchers from using Behaviors and/or Events over polluting AppTable.php / AppEntity.php. Does not enforce refactoring into Behaviors, traits, keeps upgrading 2.x to 3.x apps easier.

Related:
Reintroducing AppEntity.php is one more reason to rename standard Entites to FooEntity.php instead of simple Foo.php.

Having AppTable and AppEntity clearly shows off the new features of CakePHP 3 vs CakePHP 2 to newbies and switchers e.g. the App prefix reminds them off AppModel.php while keeping easy ways to extend all Tables/Entities.

As for Behavior management across apps
Maybe there should be a way to register behaviors (and components, helpers) to be loaded and to unregister them to not be loaded so that they should only be loaded right after initialize() is called (via events)?

This way if you call parent::initialize() and then unload (or rather unregister) a component, it will never fetch the php files nor instantiate any object.

@AD7six
Copy link
Member

AD7six commented Aug 28, 2014

There's a big problem with this suggestion and it's the implicit assumption that all tables and entities will extend App*. Outside of an app (i.e. plugins) that's not true and moreover the app namespace is a variable, so not-app classes can't use them anyway.

It really isn't a chore to create src/Model/Table/Table.php and/or src/Model/Entity/Entity.php and use them if you want to use inheritance instead of composition.

I don't understand the "behavior management across apps" point. If a table uses a behavior that you want to unbind before it does anything - it probably shouldn't be bound by default. This is possibly a hint as to why composition over inheritance is a better idea.

@markstory
Copy link
Member

I feel this is the kind of thing that can easily be done by people that need/want it. The App* were a huge headache in 2.x and that is one of the many reasons they don't exist in 3.x.

@markstory markstory added this to the 3.0.0 milestone Aug 28, 2014
@ionas
Copy link
Contributor Author

ionas commented Aug 28, 2014

So the path will rather be remove AppController as well (in 3.1? 3.2?)?

@ionas ionas changed the title 3.0 RFC: Reintroduce 'AppModel' / Related: Renaming 'Post.php' to 'PostEntity.php' 3.0 RFC: Reintroduce 'AppModel' / Related: Renaming 'Post.php' to 'PostEntity.php' (Consistency across MVC domains) Aug 28, 2014
@ionas ionas changed the title 3.0 RFC: Reintroduce 'AppModel' / Related: Renaming 'Post.php' to 'PostEntity.php' (Consistency across MVC domains) 3.0 RFC: Reintroduce 'AppModel' / Related: Renaming 'Post.php' to 'PostEntity.php' (consistency across MVC domains) Aug 28, 2014
@markstory
Copy link
Member

I still think AppController provides enough value to warrant its inclusion. Most AppModel methods I used were hacks around the old ORM's limitations.

@ADmad
Copy link
Member

ADmad commented Aug 28, 2014

My vote also is to not add or remove any App* class in 3.0. What we have currently is fine.

@ionas
Copy link
Contributor Author

ionas commented Aug 29, 2014

So I will close this. Maybe the issue will be revisted in 3.1 (e.g. making things consistent by removal off AppController ... or reintroduction of AppTable and AppEntity. Just to be on the safe side I do think moving to Entity postfix for entities is a good call, but NVM).

@ionas ionas closed this as completed Aug 29, 2014
@JD-Robbs
Copy link
Contributor

I'd have thought inheriting from one's own AppX classes could provide value in any case. Not everyone necessarily abuses it. 😉 ... That said, one can just inherit from one's own classes.

@Oxicode
Copy link
Contributor

Oxicode commented Jul 12, 2015

👍

@beporter
Copy link
Contributor

This just came up in one of our projects. We're adding a CreatorModifierBehavior that mirrors the Timestamp Behavior except for creator_id and modifier_id instead of timestamps. When I saw the PR included a new line in every single Table's initialize() for $this->addBehavior('CreatorModifier.CreatorModifier');, it felt like a pretty big step back from Cake 2 to have to modify 20-some-odd classes by hand, but hey, progress! 😆

No slight against the core team though; they made the call they thought was best. We'll be implementing our own AppTable, modifying our app skeleton to provide it out of the box, and updating our bake templates to produce Table classes that reference it. It's just unfortunately a lot of extra work we didn't have to think about with 2.x. The "simple" path is more complex then it used to be.

@dakota
Copy link
Member

dakota commented Jul 21, 2015

The way that we do this is to catch the global Model.initialize event in bootstrap.php.

\Cake\Event\EventManager::instance()->on('Model.initialize', function (\Cake\Event\Event $event) {
    $event->subject()->addBehavior('CreatorModifier.CreatorModifier');
});

BTW, the plugin I'd recommend for recording creator_id and modifier_id is https://github.com/UseMuffin/Footprint

@beporter
Copy link
Contributor

@dakota Thanks for the pointer. Ours is a crude version of that, but it'll do for now. If we outgrow it, it's good to have an alternative lined up.

I recognize the App* classes might have been a pain for the core team, but AppModel::$actsAs = ['CreatorModifier']; was notably easier for us Cake devs than hooking events in a completely unrelated part of the app (bootstrap) or having to roll your own AppTable implementation by hand. I reiterate: I'm finding that the simple path for a fair number of common (subjective, yes) things is more complex now. 😞

@lorenzo
Copy link
Member

lorenzo commented Jul 21, 2015

@beporter not only for the core team, I found those classes error prone for everyone else. Do you remember all the times you had to make an exception for a plugin model or any other model because the AppModel was being "too smart"?

What is wrong with having your own AppTable, btw?

@beporter
Copy link
Contributor

@lorenzo :waves hand: This isn't really the place for that. I'm happy to have the conversation in Slack or irc or email (I have no shortage of opinions 😉), but we don't need to pollute this thread with a meta conversation. I have a path available to me to resolve the issue, and my team and I will use it-- nothing more needs to be said here. Thanks all.

@ionas
Copy link
Contributor Author

ionas commented Jul 21, 2015

@beporter I'd have loved to see a log of that conversation to see some perspectives.

@markstory
Copy link
Member

I don't see how having a core provided AppTable is any better/different than implementing it on an as needed basis in applications.

@beporter
Copy link
Contributor

I've written four different drafts trying to come up with an approach that isn't going to start a much, much deeper discussion and all four are abject failures at that.

@lorenzo, @markstory: I can't address your questions here, even if I feel like I might have a case to make. I'm willing to just let it go for now.

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

No branches or pull requests

9 participants