In one particular case, the migrate_extras module. The class (migrate_extras/entity_api.inc) is used by common distributions such as Commerce Kickstart.

In sps.module... the following seems to be the problem:

223 /**
224  * Implements hook_entity_info_alter().
225  *
226  * changing the node controller on behalf of the node_load Reaction
227  */
228 function sps_entity_info_alter(&$entity_info) {
229   foreach($entity_info as $name => &$info) {
230     if(isset($info['revision table'])) {
231       $info['controller class base'] = '\Drupal\sps\EntityController\\' .$info['controller class'];
232       $info['controller class base'] = $info['controller class'];
233       $info['controller class'] = '\Drupal\sps\EntityController\EntityControllerWrapper';
234       $info['creation callback'] = 'sps_entity_create';
235     }
236   }
237   //$entity_info['node']['controller class'] = '\Drupal\sps\EntityController\NodeController';
238   //$entity_info['bean']['controller class'] = '\Drupal\sps\EntityController\BeanEntityAPIController';
239 }

Some questions:

  1. Why is $info['controller class base'] being set twice? Wouldn't the second setting override the first?
  2. $info['controller class'] = '\Drupal\sps\EntityController\EntityControllerWrapper'; - this line appears to be the breaking point where entity_save no longer works for the other modules.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

indytechcook’s picture

Not sure why it's double set but the point of the EntityControllerWrapper is to override the load for all entity controllers. Every other method is deferred. http://drupalcode.org/project/sps.git/blob/refs/heads/7.x-1.x:/lib/Drupa...

migrate_extras/entity_api.inc)

This is a file and not a class. Can you please provide more information around this.

- this line appears to be the breaking point where entity_save no longer works for the other modules.

This was tested with other modules that provide their own entities.

Can you perhaps elaborate on what's going on? Perhaps there is a race condition between commerce kickstart and sps. Perhaps we need to change the sps project weight to go last.

saltednut’s picture

This is a file and not a class. Can you please provide more information around this.

Sure... its a migrate class inside the file here: http://drupalcode.org/project/migrate_extras.git/blob/HEAD:/entity_api.inc

I believe we're concerned with what's going on here:

 219       // Create a real entity object, update its values with the ones we have
 220       // and pass it along.
 221       $new_entity = array();
 222       if (!empty($this->bundle) && !empty($this->info['entity keys']['bundle'])) {
 223         $new_entity[$this->info['entity keys']['bundle']] = $this->bundle;
 224       }
 225       $new_entity = entity_create($this->entityType, $new_entity);
 226       foreach ($entity as $field => $value) {
 227         $new_entity->$field = $entity->$field;
 228       }
 229 
 230       // If a destination id exists, the entity is obviously not new.
 231       if (!empty($new_entity->{$this->id}) && isset($new_entity->is_new)) {
 232         unset($new_entity->is_new);
 233       }
 234       $entity = $new_entity;
 235       $this->prepare($entity, $row);
 236     }
 237 
 238     $updating = (!empty($entity->{$this->id}) && empty($entity->is_new));
 239 
 240     migrate_instrument_start('entity_save');
 241     entity_save($this->entityType, $entity);
 242     // It's probably not worth keeping the static cache around.
 243     entity_get_controller($this->entityType)->resetCache();
 244     migrate_instrument_stop('entity_save');

I tried changing to sps_entity_create($this->entityType, $new_entity); but this did not fix it.

The only thing I have been able to do is remove the EntityControllerWrapper override (commented it out) and then the migration is able to save the entities. But clearly this would prevent SPS from working.

saltednut’s picture

FileSize
1.3 KB

237 //$entity_info['node']['controller class'] = '\Drupal\sps\EntityController\NodeController';
238 //$entity_info['bean']['controller class'] = '\Drupal\sps\EntityController\BeanEntityAPIController';

Has SPS been tested on other entities besides Node and Bean?

Attached is a band-aid, not a real patch... wondering if it does enough to prevent the takeover of non-node, non-bean entities.

ericduran’s picture

Status: Active » Reviewed & tested by the community

This needs to get in.

It essentially breaks every other entity.

:)

ericduran’s picture

So, I RTBC this patch because it does make things a bit more usable.

That being said I don't think this is the ideal patch FYI.

In anything I'll like to have a white-list of classes that are know and tested.

This way we can add other Entity support. I'm currently working on proper Entity API integration. FYI.

Anyways that's probably going to be a follow up issue.

Just wanted to mention that.

ericduran’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/sps.moduleundefined
@@ -227,15 +227,15 @@ function sps_query_alter($query) {
+      if ($info['controller class'] == 'NodeController' || $info['controller class'] == 'BeanEntityAPIController') {

Lets declare a supported controller list somewhere.

So we can just add a new controller to the list as we support more Entity.

This way we can do a simple in_array() or something to simplify the addition of new controllers.

The if() statement will get ugly pretty quick,

saltednut’s picture

@ericduran yes that makes sense. I am not sure where to store this list though. Recommendations?

indytechcook’s picture

Has SPS been tested on other entities besides Node and Bean?

We tested it with fieldable panel panes.

indytechcook’s picture

@ericduran yes that makes sense. I am not sure where to store this list though. Recommendations?

We were tryign to put all higher level config into a config controller which was retrieved by sps_get_default_config_controller(). So basically it's a variable.

ericduran’s picture

Yep, I know it breaks on certain entities but I'm aware the goal of the module is not really to white list which is why I've not committed anything like that.

:-) I've taken the lets just fix any broken entities approach.

saltednut’s picture

Status: Needs work » Needs review
FileSize
853 bytes

Looks like this is still a problem. I've rerolled the band-aid and added a whitelist array to make things a little cleaner. I'm also trusting FPP on @indytechcook's word. :)

Elijah Lynn’s picture

Brant, did you mean for the patch #11 to apply on top of #3?

ericduran’s picture

So I've held up on holding applying this patch even thought we run in in our environment mainly because I don't think that's the goal of the project.

The project doesn't want to be whitelisted it wants to work with as much entity as possible without blowing up.

As I see this is a temporary workaround patch which is why I've help from committing it.

If everyone disagrees with me then I'm ok merging it.

ericduran’s picture

Ok, this is actually hurting us more than is helping us.

I'm going to proposed a simple solution:

Custom entities that support sps should add the

'previewable' => TRUE,

Key in their entity info.

This will let us know which entities are ok to alter. We can keep exceptions for nodes & beans.

I would tell you we have custom entities that support sps and it is a bit of a hassle to maintain this patch. I rather fix it with a solution that works best for everyone :-)

saltednut’s picture

I think this is a great idea. Perhaps SPS can provide an alter for the node entity that adds the 'previewable' key?

I'd be happy to commit a patch to Bean that added this key too.

+11111

ericduran’s picture

FileSize
2.24 KB

Here's a 1st pass. I'm probably going to deploy this to a big site so I'll have feedback rather soon ha.

ericduran’s picture

Status: Needs review » Fixed

I went ahead an merge the last commit.

This is working fine.

  • Commit be9bd83 on 7.x-1.x by ericduran:
    Issue #1934130 by brantwynn, ericduran: SPS should not prevent other...
indytechcook’s picture

While I understand the need, it makes me sad. That was the opposite intention of SPS.

saltednut’s picture

Status: Needs review » Fixed

Well there is fantasy and there is reality. I'm a proponent of solutions that fit reality, so I'm not sad.

Its just reality that not all entities work the same way in Drupal. If all entities could be treated by core like first class citizens, such as node then the theory you refer to would have worked.

Cest la vi.

Let's work on something better for D8.

ericduran’s picture

Yea, I completely understand.

After working on a bunch of SPS implementations this was literally the thing that just kept coming up. It's hard to justify blowing up a site over a trivial patch :-/

I do think it's extremely easy for us to add support now we just really need to test and mark it.

Status: Fixed » Closed (fixed)

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