Support from Acquia helps fund testing for Drupal Acquia logo

Comments

robinvdvleuten’s picture

Attached patch

Status: Needs review » Needs work

The last submitted patch, bean-features-integration-1984934-1.patch, failed testing.

DamienMcKenna’s picture

Title: Added integration with Features » Make Beans exportable with Features

Clarifying the title.

Iztok’s picture

I see that at #1849668: Add support for Bean exporting (deployable bean instances). there is a patch for UUID_features that enables exporting Beans.

I tested you patch but does not seem to be importing - I can help with testing but first I would need to know which of those two solutions should be implemented?

saltednut’s picture

There is little movement on getting the patch we worked on into uuid_features. At this time, I am using that patch in projects, but I'm open to keeping this one alive if robinvdvleuten would like to continue working on it. I'll come back here for a code review of #1 soon.

robinvdvleuten’s picture

I will try to get this patch working. Because I prefer to have the export functionality inside the module itself, instead of having a module which contains functionality for multiple modules. I need this functionality in a project I'm currently working on, so I'll try to have the patch asap.

jonhattan’s picture

robinvdvleuten’s picture

Sorry for my extreme late response on this issue. I've updated the patch so all beans will be exported AND imported. I think deploy support is nice, but in many cases you just want to include the created beans with features (on most sites I've worked on I never used the deploy module).

robinvdvleuten’s picture

Status: Needs work » Needs review

I should bump the status to 'needs review'.

Angry Dan’s picture

Why does this need to have anything to do with uuid? Since beans already have a machine name can't they be made exportable using that as the key?

I thought that the entity API had tools to deal with that kind of thing. I'm pretty sure that some flag can be set somewhere to make the entity exportable. Then all you'd want is a features alter hook to prevent altered beans making a feature show as overridden?

Excuse my ignorance if I'm wrong - I don't know a great deal about the mechanics of features and the entity API, but I do know from past experience that the UUID module is a pain. And it seems odd to me that you would need two cross-machine methods of uniquely identifying a bean?

saltednut’s picture

The UUID is necessary to help ensure uniqueness. I don't think it would be technically required, but since bean_uuid exists it might be advantageous to use uuids.

One example I can think of:

A site has an existing bean with the machine name of 'news'

A feature is installed with an exported bean that has the machine name of 'news'

This could cause a problem with the new 'news' bean overwriting the old one. That might be the desired use case for some, but not always.

Using UUID would ensure that any beans injected into the system with Features are unique and can be reverted.

However, in the UUID scenario, I could see a Bean being "stuck" as overridden if the machine name it is asking for via Features export is already in use.

Angry Dan’s picture

Brantwynn, I think you are confusing distinct problems and introducing an unnecessary dependency on UUID. If I have a content type on my site called "blog", and then choose to install a feature called "blog" then I'm going to run into issues. Would you suggest that content types should have a UUID? Wouldn't that make the machine name field redundant?

It's the same here, and it's a simple a fact of Drupal because it regularly uses machine names as a user-friendly unique identifier. Either Bean should switch to a "bid" field and drop the machine name (which would be a shame, but would justify the use of a UUID field) or it should trust users to manage the unique machine names of their beans.

I appreciate your concern, but in the real world I simply can't see beans being exported into contrib features, making the whole of this a moot point, since site builders are highly unlikely to want to have two beans with the same machine name and different UUIDs.

My advice is definitely to separate the UUID concern out into the bean_uuid module (which I admit I've not looked at). Also, I'd remind of you KISS - keep it simple, stupid. Don't add unnecessary complexity that's not essential; by your own admission you "don't think it would be technically required". If that is the case, don't add it, but by all means build extensibility in, then the bean_uuid module will be free to pick up where you left off.

Eric_A’s picture

This is the same patch except that it exports block delta... No more errors on feature revert.
In the mean time the UUID Features project has done quite some work, though. I haven't looked into that yet.

Eric_A’s picture

Ok, #13 prevents the errors during installation and a normal feature-revert and also the duplicating of beans from #12, *but* drush feature-revert --force will lead to WD bean: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'blah' for key 'delta'.

To me it seems that this is actually needs work.

saltednut’s picture

I think you are confusing distinct problems and introducing an unnecessary dependency on UUID. If I have a content type on my site called "blog", and then choose to install a feature called "blog" then I'm going to run into issues. Would you suggest that content types should have a UUID? Wouldn't that make the machine name field redundant?

Hi Angry Dan,

I think we're having a communication issue here. In #11 I am talking about individual blocks (beans, whatever). Not the block type. When I say:

A site has an existing bean with the machine name of 'news'

I mean: A site has an existing BEAN. A Bean is the individual instance. In Node terms, I'd be talking about a NODE, not a content type.

We are talking about exporting Beans (ie: specific instances of a Bean, not Bean bundles (ie: Beans version of a Content type) aka Block Types.

A bean bundle would not need a UUID. I never said it would. A bean bundle is often created as a plugin. Other times it is created via the Bean Admin UI. If we are talking about Block Types created by the Bean Admin UI, then no, you don't need UUID. Block Types created as plugins don't need to be exportable because they are already contained within code. Correct me if I am mistaken, but I believe the purpose of the patch being worked on is to export the actual Beans themselves, not the Block Types created by the Bean Admin UI.

An individual instance of a bean should have a unique identifier. This is so that you can ensure you are deploying and versioning always the correct bean. We can't rely on machine names (they may be redundant), nor can we rely on BID, since that is an int value that will be different from site to site. So in order to deploy ANYTHING that is content, whether it be a Node, a Bean, a Taxonomy term, etc - you need a UUID. This is what UUID_Features has set out to solve. We did a lot of work already solving this problem with UUID Features. mpgeek and I worked on a patch months ago that solves this problem.

The patch in #13 essentially is providing a fragmentation of that solution. However, it is a native solution that does not require UUID Features and is also a bit lighter code, so thats why we're working on it. If you need to export individual Beans right now, you can install Bean, Bean UUID and UUID Features and have that ability. The goal of this issue is to make it so that simply by installing Bean and Bean UUID, you would have the ability to deploy individual beans, thereby removing the need for UUID Features and carrying less code bloat. ie: KISS

But you still need UUID's to have unique bean deployment.

It would be nice to have a followup here to allow for the exporting of Block Types created via the Bean Admin UI. This would be the same thing as exporting a Content Type in Features except for Beans, which is the use case described in #12.

Regarding #14, we ran into this issue already with UUID Features. The bean with that delta already exists so you can't save it as a new bean. You have to insert logic to either load and update the bean or save it new depending on what you are trying to do. Check out http://drupalcode.org/project/uuid_features.git/blob/refs/heads/7.x-1.x:...

saltednut’s picture

  1. +++ b/bean_features/bean_features.features.inc
    @@ -0,0 +1,126 @@
    +/**
    + * Implements hook_features_rebuild().
    + */
    +function bean_features_rebuild($module_name) {
    +  $bean_defaults = module_invoke($module_name, 'bean_features_default_beans');
    +  foreach ($bean_defaults as $bean_default) {
    +    $bean = bean_create($bean_default);
    +    $bean->save();
    +  }
    +}
    

    You need to load an existing bean and update it with the new values OR create a new bean here.

  2. +++ b/bean_features/bean_features.features.inc
    @@ -0,0 +1,126 @@
    + * Implements hook_features_revert().
    + */
    +function bean_features_revert($module_name) {
    +  $bean_defaults = module_invoke($module_name, 'bean_features_default_beans');
    +  foreach ($bean_defaults as $bean_default) {
    +    $bean = bean_create($bean_default);
    +    $bean->save();
    +  }
    +}
    

    This code is redundant. Just call bean_features_rebuild($module_name) inside of bean_features_revert()

saltednut’s picture

Status: Needs review » Needs work
Manuel Garcia’s picture

Here's my attempt at #16.1, and addressing #16.2

The feature after reverting it still shows as overridden, showing the title_original in OVERRIDES, but not in DEFAULT.

gordon’s picture

I have moved this into the bean module. Since features allow us to use a separate file for the code so it only had 1 additional function compared to loading an entire module if you are using the module.

Status: Needs review » Needs work

The last submitted patch, 19: make_beans_exportable-1984934-19.patch, failed testing. View results

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
4.73 KB

Rerolled.

DamienMcKenna’s picture

FYI the benefit of having this as a submodule is that every site wouldn't immediately show up with all of their bean objects available for export, which could be quite a lot of items for some sites, and it could drastically affect site performance in certain circumstances.

shuchisethi’s picture

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/bean-n1984934-21.patch

nironan’s picture

The patch at #21 applies cleanly against bean v. 1.13