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 "callback" functions from hook_entity_info() #164

Closed
quicksketch opened this issue Jan 28, 2014 · 6 comments
Closed

Remove "callback" functions from hook_entity_info() #164

quicksketch opened this issue Jan 28, 2014 · 6 comments

Comments

@quicksketch
Copy link
Member

Since we've converted our entities to real classes, we no longer have need of the callbacks in hook_entity_info(), including the uri callback and the label callback. Instead, these can just be methods on the entity classes themselves.

Continuing that line of thinking, we also don't need to store any entityInfo in individual entity objects. This information was used to pull out the callbacks, but if we don't have the callbacks, we don't need the information any more.

And finally, if we don't need the entityInfo property any more, we don't need the magic __sleep() and __wakeup() methods.

Removing the magic methods and reducing the callstack of labels and uri's should provide a negligible performance improvement, but it will significantly simplify the call stack for developers.

@rudiedirkx
Copy link

You need some entityInfo. I'd keep entity type and bundle (as a method, because it can change). If you want any generic entity handling, all entitiy objects need a type.

@quicksketch
Copy link
Member Author

Hi @rudiedirkx, you're right that we'll need a way to get the entity type and bundle. For now I've done exactly that and left those as properties but everything else requiring entityInfo has been moved to methods within each entity type. The work is currently at https://github.com/quicksketch/backdrop/compare/164;remove_entity_callbacks (but it's dependent upon the File Entity PR at #152, so the first 3 commits are from that issue).

However, we could remove entity type and bundle properties as well if we made those methods in each entity type class. For example the Node entity could simply return it's type like this:

class Node extends Entity {
  public function getEntityType() {
    return 'node';
  }
  public function getBundle() {
    return $this->type;
  }
}

The getEntityType() and getBundle() methods would be declared abstract in the Entity class.

This would have the advantage of making it so that Entities contained no private/protected variables and the only thing contained in a subclassed Entity object to be the data itself.

@rudiedirkx
Copy link

I like that! The current D7 Entity goes way overboard, storing way too much info in every entity object. No idea how D8 does. This is much better indeed. I'd keep the getEntityInfo() method though, which lazyloads from cache.

@quicksketch
Copy link
Member Author

I've updated my PR at https://github.com/quicksketch/backdrop/compare/164;remove_entity_callbacks to remove the entityType and entityBundle properties as well. Now when you debug() or print_r() an Entity you get the gloriously simple output of only the data and no internal entity properties. That makes managing and inspecting entities quite a bit simpler.

For now I've left out the getEntityInfo() method, since it'd only be a wrapper around get_entity_info($this->entityType()). Hopefully we'll be able to provide methods directly in the classes that make any use of the raw entity info completely unnecessary. I'd love the have your input @rudiedirkx on the implementation.

@quicksketch
Copy link
Member Author

I filed an official PR at backdrop/backdrop#133. This looks like a good improvement to our code clarity and developer experience working with entity objects.

@quicksketch
Copy link
Member Author

In the interest of moving forward on our new Entity implementation, I've merged in that PR to simplify the entity objects. Still love to hear your feedback @rudiedirkx if you get a chance to review. Thanks!

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

2 participants