This module right now is 80% awesome and 20% sheer madness and the latter is faces. I can't see why do we need it. We are layering some meta OOP on top of the already broken PHP OOP -- and we know that all these things are futile. Please don't. I know you try to be elegant etc but in fact you are making the module convoluted and impossible to follow.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

I agree that it was a bad idea to add into the entity API project at all, as I would not suggest anyone to use it as the entity base class.
It's pretty much there as I use it for Rules, as it uses it to implement its action detection mechanism, which I'd really love to scratch in favour of just having a single class per action - which would benefit from the autoloading mechanism. --> D8, for D7 I felt like this would break too much with the way the people are used to write actions. I should have changed that anyway.

However, I see no cause to not scratch it here + let it Rules implement on its own.

klausi’s picture

Status: Active » Needs review
FileSize
15.43 KB

Yep, Faces is powerful, but only necessary for very advanced purposes. On the downside it adds a lot of complexity, so let's remove it from Entity to keep it as simple as possible. Patch attached.

fago’s picture

Status: Needs review » Needs work

thanks. I think we'll also need to add in classes to retain BC for modules having used the faces variants (but actually never used faces).

I've just added faces.inc to Rules, to make sure its in place there when it's gone from the entity API.

sun’s picture

Status: Needs work » Reviewed & tested by the community

Even though there seems to be a beta release already, this module actually is still pre-alpha right now, so screw BC.

fago’s picture

Status: Reviewed & tested by the community » Needs work

I'm fine with that change, as I assume no-one except me actually really uses the faces API in a custom module. Still I think there are some modules that just use the faces class without actually using faces (I think search_api does) - for them we need to provide the class EntityDBExtendable just extending Entity, for BC.

klausi’s picture

Status: Needs work » Needs review
FileSize
15.95 KB

Ok, keeping 3 lines of code with that old classes.

klausi’s picture

Grammar mistake: "These" instead of "This"

fago’s picture

Status: Needs review » Fixed

thanks, committed.

klausi’s picture

Status: Fixed » Reviewed & tested by the community

I don't see the commit, did you push it?

fago’s picture

Status: Reviewed & tested by the community » Fixed

grml, CVS $Id$ let the Git cvs export script fail. I worked around that and finally committed it.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.