Support from Acquia helps fund testing for Drupal Acquia logo

Comments

colan’s picture

colan’s picture

Title: Add general entity support » Add generic entity support

Marked #1337408: Not compatible with 'Field Collection' as a duplicate of this issue.

colan’s picture

Marked #1352270: Support for Field Collections as a duplicate of this issue.

colan’s picture

Priority: Normal » Major

Bumping this up to major, as it's collecting a lot of duplicates. Here's another: #1469412: serial field does not work in user table

kaizerking’s picture

waiting for this feature where i can assign serial number to user

drvdt’s picture

Assigned: Unassigned » drvdt

Why only for node?
This is a great module that needs for many entities. Please improve it.
Is any body know an other same module?
Many thanks,

colan’s picture

Assigned: drvdt » Unassigned

@drvdt: Please do not assign the ticket to yourself unless you plan on working on it.

As the current maintainer, I do not have the time to work on this right now. I only needed it for nodes, but yes, this needs to be generalized. Someone else will have to provide a patch, or pay someone to work on it. Once it's sufficiently tested, I'll commit it.

MM10’s picture

Colan-- Any broad suggestions or pointers on how to best generalize it?

colan’s picture

This is really broad, but search for "node" or "nid" in the code, and then replace with "entity" or "entity_id" respectively. ;) You may need to store the entity type and the entity ID in each table as opposed to just the node ID, but I haven't looked at it that closely in a while. Also, take a look at the field API.

The code isn't that dense, so it shouldn't be too hard to take a look and see what needs doing. Play with it, and see what happens. Using other field modules (that have generic entity support) as examples would be extremely beneficial here.

drvdt’s picture

Did any body try #9?

andyhu’s picture

+1 for this feature, I think in order to get it working for all entities, we need to identify the entity type and find the base table using entity_get_info(), it shouldn't be too hard :)

j4’s picture

I did. But I am a total newbie to php, so am afraid i did not make any progress. But would certainly love to have this feature.

Thanks
Jaya

kaizerking’s picture

@j4, What you did?tried on entities?, please post it here we will test

j4’s picture

I did precisely what was offered as a tip in #9, but it threw up all kinds of errors. Sorry i did not document the errors!

Jaya

kaizerking’s picture

I suggest go through the entity API to see how an entity behaves on entity_create,entity_save,entity_update,entity_delete,I am not coder, so I cannot code, I can understand a bit on entity_hooks.

j4’s picture

Will report back if i succeed..thanks

J

lukus’s picture

I'm interested in using the serial field with the field collection module.

I'd be willing to attempt to convert the module to work with entities if nobody else is working on it?

colan’s picture

@lukus: It's open for you to assign to yourself, and start working on it. Thanks! Looking forward to your patch. :)

lukus’s picture

Assigned: Unassigned » lukus

@colan - I'm going to give this a go, I'll report back later today with any update.

lukus’s picture

Assigned: lukus » Unassigned

@colan

Okay I've started working on this. I've looked over the code, and have produced an overview of functionality. I think I understand what's going on.

It doesn't seem like a huge job - as much of the code already works with the Field API. The main issue occurs when a serial field is applied to an entity with pre-existing content; so I'm going to try to provide a generalised function that takes the place of _serial_init_old_nodes() to generate sids for any entity with pre-existing content.

One other point I've considered;
Field collection entities are generally used with a 'host entity'. Bearing this in mind, eventually I think we might need to be able to choose whether serials are allocated locally or globally. I think this could possibly be done via field UI settings.

ie.

  • locally: sid is unique to each new instance of a field collection (e.g. '1... n' for each host node/entity).
  • globally: sid spans all instances of a field collection bundle (e.g. '1... n' across all host node/entity).

For now I'm going to just work on the latter case, as that's what I currently require.

Does is sound like I'm going in the right direction?

lukus’s picture

Assigned: Unassigned » lukus
lukus’s picture

Here's my patch.

I've tested it with nodes, field collections, users and taxonomy vocabularies - both empty and pre-populated bundles

It's functional, but there's still one thing left to do:

  • if an entity's machine_name is changed the table name isn't updated automatically - e.g. a taxonomy vocabulary. Need to find the relevant hook for this.

I'd strongly recommend not using this on any production site yet - it's very much alpha. If someone could review the code for me, and try it out on a test site, I'd be most appreciative.

I'm going to look over it again tomorrow morning and test it more thoroughly.

lukus’s picture

Status: Active » Needs review
colan’s picture

First of all, thanks so much for taking this on.

if an entity's machine_name is changed the table name isn't updated automatically - e.g. a taxonomy vocabulary. Need to find the relevant hook for this.

How about hook_field_attach_rename_bundle?

The basics of what you've got make sense to me.

There are several coding standards violations, but I'm assuming that these will get cleaned up when you're done. It seems like you're not done because some lines are commented out - these need to disappear in the final version. Also, we need to figure out if that drupal_goto() should stay or go. I'm not sure myself.

I'm not in a good position to test this right now so let's wait and see if anyone else can test and/or comment on the above.

lukus’s picture

Cheers Colan,

I'll work on the generic rename function later today, and I'll also clean up the code so it passes review standards. Will add a new patch once it's done.

lukus’s picture

Hi

I've refactored the code to meet drupal.og coding conventions and I've also added the new generic hook to deal with renaming entity machine names / serial table names.

Could someone please review?

kaizerking’s picture

@lukus nice work

also it would be nice if we have the following
1.a specific number range for each entity type
2.assigning the number range for specific period
3.after expiration of the specific period the assigned number range cannot be used
4.allow prefix to number range ex:AA-12345 or AA/12345
5.allow suffix to the number range 1234/2012
6.Lock:if the same type of entity is being created by another user ? so the serial number shouldn't be assigned unless saved
to achieve the above we need a configuration settings for creating number ranges
hope some one will patch

lukus’s picture

@kaizerking

Thanks for the feedback. Can you confirm whether the patch functions as expected?

Re. the feature additions, I'd recommend adding a new issue/s and to mark them as 'feature requests'.

kaizerking’s picture

I am getting this error when i am trying to create serial field on user accounts
http://localhost/xxxxxx/#overlay=admin/config/people/accounts/fields

There was a problem creating field test: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '' for key 'name'
How ever Serial number is created when i create user. the number is not wrong indexing
before creating the user i have total 4 user including user 1
after enabiling serial module i have created one user. but the user number is generated as 2

lukus’s picture

@kaizerking

Thanks for for trying out the patch - much appreciated.

With the User entity, user '0' (anonymous) has no element for the name key by default and this is causing problems in the _serial_init_old_entities() function. For this specific case I think the count needs to begin from the first non-anonymous user.

I'll refine the code and release a new patch later today.

lukus’s picture

Here's the new patch. I've added an exception for the anonymous user - the anonymous user isn't given a serial.

@kaizerking - could you please apply the new patch and delete your serial field (and recreate it). Should be okay now.

kaizerking’s picture

Nice,patch @ #31 worked for me, thanks

lukus’s picture

Thanks kaizerking.

Can anyone else provide any feedback?

PipB’s picture

With patch #31 I still have this error:
Notice: Undefined property: stdClass::$nid in serial_field_insert() (line 83 of /home/xxxxx/domains/xxxxx.xx/public_html/sites/all/modules/serial/serial.module).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'nid' cannot be null: INSERT INTO {serial_user_field_lidnr_} (nid) VALUES (:db_insert_placeholder_0); Array ( [:db_insert_placeholder_0] => ) in _serial_generate_value() (line 155 of /home/xxxxxxx/domains/xxxxx.xx/public_html/sites/all/modules/serial/serial.inc).

PipB’s picture

I implement the patch from #31 and on another website I insert the serial field into an account, I got this error:
Fatal error: Call to undefined function _serial_init_old_nodes() in /home/xxxxx/domains/xxxxxx.xx/public_html/sites/all/modules/serial/serial.module on line 37

ytsejam’s picture

Applied the patch and got the same error while trying to make a new user account with a serial field:

Notice: Undefined property: stdClass::$nid in serial_field_insert() (line 80 of /xxx/sites/all/modules/serial/serial.module).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'nid' cannot be null: INSERT INTO {serial_user_field_usernumber} (nid) VALUES (:db_insert_placeholder_0); Array ( [:db_insert_placeholder_0] => ) in _serial_generate_value() (line 155 of /xxx/sites/all/modules/serial/serial.inc).

Any ideas? Thanks in advance.

lukus’s picture

Hi .. I'll be working on this later today.

Just to confirm, the error occurs on creation of a new user account?

ytsejam’s picture

Yes. And after the error, a new user is not created. Looking forward to an update, thanks in advance!

serguitus’s picture

No new results yet?

HLopes’s picture

There's an hardcoded $entity->nid, that obviously isn't set on a user object.

Should work if you use entity_extract_ids to get the entity id instead.

function serial_field_insert($entity_type, $entity, $field, $instance, $langcode, &$items) {
    module_load_include('inc', 'serial');
    $ids = entity_extract_ids($entity_type, $entity);
    $sid = _serial_generate_value($ids[0], $instance['bundle'], $field['field_name']);
    $items = array(array('value' => $sid));
    $entity->$field['field_name'] = $items;
}
ikeigenwijs’s picture

Is there progress? is this rolled in the .dev?

hoporr’s picture

Marked https://drupal.org/node/1552280 Profile Serial field - Undefined property: Profile::$nid as duplicate of this.

avalot’s picture

Very interested in progress on this issue. I need a "member number" field on the user entity, and it's not working right now. How can I help?

tien.xuan.vo’s picture

Here is my patch, base on #31 's patch.

Changes:

  1. Add entity_type to serial tables.
  2. Use entity_get_info and db_query instead of EntityFieldQuery.

Some notes:

  • I can't find serial_field_insert(). I think it was removed from latest code.

Tested with:

  • Node.
  • Entity types from ECK
  • User.

Please review my patch. Sorry for my English!

tien.xuan.vo’s picture

Changes:

  1. Add tokens support for generic entity.
PROMES’s picture

It works for me.
Thanks

joachim’s picture

Status: Needs review » Needs work

This would be a really great improvement!

Patch needs a bit of work though -- mostly just tweaks, though the big think I'm not sure of is adding the dependency on entity module. That's quite a big change!

  1. +++ b/serial.inc
    @@ -170,48 +174,67 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
    +  $bundle_key = $entity_info['entity keys']['bundle'];
    
    +++ b/serial.install
    @@ -84,3 +84,15 @@ function serial_update_7130() {
    +    $tablename_without_serial = preg_replace('/^serial_/', '', $tablename);
    +    db_rename_table($tablename, 'serial_node_' . $tablename_without_serial);
    

    I don't really follow why you're stripping the initial 'serial_' here. If you're using preg_replace() anyway, just do '/^serial_/' to 'serial_node_'.

    (Or be ultra-swanky and use a lookbehind: '/(?<=^serial_)/' to 'node_'.)

  2. +++ b/serial.inc
    @@ -170,48 +174,67 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
    +      $entity = entity_load_single($entity_type, $id);
    

    Use entity_load() to avoid a dependency on entity module.

  3. +++ b/serial.inc
    @@ -170,48 +174,67 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
    - * @return result set containing pairs of (node type name, field name).
    + * @return result set containing of (entity type, entity bundle, field name).
    

    This didn't make that much sense beforehand, but it makes even less now.

  4. +++ b/serial.info
    @@ -2,6 +2,7 @@ name = Serial
    +dependencies[] = entity
    

    Do we need to add this dependency? I've only found one use of a function from that module -- entity_load_single(), which can be dispensed with.

  5. +++ b/serial.module
    @@ -68,10 +68,6 @@ function serial_form_alter(&$form, $form_state, $form_id) {
    -
    -    // Go back to Managed Fields:
    -    $type = $form['#bundle'];
    -    drupal_goto("admin/structure/types/manage/$type/fields");
       }
    

    That looks like an unrelated fix; should be tackled in its own issue.

  6. +++ b/serial.module
    @@ -106,45 +102,67 @@ function serial_node_presave($node) {
    +  // Update the extra weights variable with new information.
    

    That comment doesn't look right. 'weights'?

ron_s’s picture

This would definitely be great to get incorporated into the module.

I've reviewed the patch and agree with @joachim on his feedback. I don't see a reason why the functionality needs to be dependent on entity. Just replace this line:

$entity = entity_load_single($entity_type, $id);

With this:

$entity = entity_load($entity_type, array($id));

... and should be able to remove the dependency. Also I agree we should just handle the preg_replace as part of one statement rather than two.

One issue I did uncover in my testing is with the serial_tokens function. It's running into a conflict because Serial and another module are both attempting to process the same token. I haven't yet figured out which module is causing the conflict, but my guess is its either Entityform or Pathauto Entity.

When saving the entity in this scenario, we see the following error messages:

Warning: Illegal offset type in isset or empty in pathauto_cleanstring() (line 180 of /sites/all/modules/pathauto/pathauto.inc).
Warning: html_entity_decode() expects parameter 1 to be string, array given in decode_entities() (line 463 of /includes/unicode.inc).
Warning: Illegal offset type in pathauto_cleanstring() (line 223 of /sites/all/modules/pathauto/pathauto.inc).

The result is no change to the entity's path. When looking at the $string pushed into pathauto_cleanstring(), I noticed it is actually an array. This is the same issue that has been documented before as part of Pathauto:

https://www.drupal.org/node/1604766
https://www.drupal.org/node/1246074
https://www.drupal.org/node/1792436

I was able to resolve this by commenting out the serial_tokens function in the patch. My recommendation would be to add some logic to serial_tokens to skip processing the code if possible conflicting modules exist.

ron_s’s picture

Attached is a new patch for review. The old patch can no longer be applied to the current -dev. Updates from the previous version:

  1. Removed dependency on Entity module by switching to entity_load
  2. Updated preg_replace in serial_update_7130 to remove the unneeded extra step
  3. Retained drupal_goto included in serial_form_alter
  4. Removed unrelated reference to "Update the extra weights" in serial_field_attach_rename_bundle
  5. Added code to serial_tokens to check if the serial field is used in a pathauto pattern. If so, skip the token replacement.

@joachim, one point I didn't understand from your previous comments was this:

+++ b/serial.inc
@@ -170,48 +174,67 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
- * @return result set containing pairs of (node type name, field name).
+ * @return result set containing of (entity type, entity bundle, field name).

This didn't make that much sense beforehand, but it makes even less now.

Can you explain further? This is related to the _serial_get_all_fields function in serial.inc. The only place I see this being referenced is in hook_schema in serial.install. Are you suggesting the serial_schema code should be removed along with _serial_get_all_fields?

I look forward to feedback so we can make any further adjustments and get this committed.

ron_s’s picture

Status: Needs work » Needs review

Changing status.

The last submitted patch, 22: serial-update_to_function_with_entities-1333032-22.patch, failed testing.

The last submitted patch, 26: serial-update_to_function_with_entities-1333032-26.patch, failed testing.

The last submitted patch, 45: serial-update_to_function_with_entities-1333032-45.patch, failed testing.

The last submitted patch, 31: serial-update_to_function_with_entities-1333032-31.patch, failed testing.

ron_s’s picture

Sorry, I had forgot to update a function reference in the previous version that did not match the original patch. Needed to change _serial_init_old_nodes to _serial_init_old_entities in the serial_field_create_instance function.

Let's try this one.

ron_s’s picture

Ok, one more update. I realized the newest -dev is using LANGUAGE_NONE rather than 'und' in _serial_init_old_nodes, which is what the patch should have too.

Have updated to include this.

The last submitted patch, 26: serial-update_to_function_with_entities-1333032-26.patch, failed testing.

The last submitted patch, 22: serial-update_to_function_with_entities-1333032-22.patch, failed testing.

The last submitted patch, 45: serial-update_to_function_with_entities-1333032-45.patch, failed testing.

The last submitted patch, 31: serial-update_to_function_with_entities-1333032-31.patch, failed testing.

jsibley’s picture

Hi. Is there any chance of getting the most recent patch, which seems to have passed, committed?

Thanks.

colan’s picture

As soon as folks have a chance to review the code and test it (and the state is RTBC), I'll happily commit this.

BR0kEN’s picture

  1. +++ b/serial.inc
    @@ -41,14 +41,14 @@ function _serial_drop_table(array $field, array $instance) {
    + * Renames serial table(s) when a entity bundle us renamed.
    

    Maybe "Rename serial table(s) when an entity bundle was renamed."?

  2. +++ b/serial.inc
    @@ -41,14 +41,14 @@ function _serial_drop_table(array $field, array $instance) {
    + * @param $bundle_old
    

    Missed param type.

  3. +++ b/serial.inc
    @@ -41,14 +41,14 @@ function _serial_drop_table(array $field, array $instance) {
    + * @param $bundle_new
    

    Missed param type.

  4. +++ b/serial.inc
    @@ -80,22 +83,24 @@ function _serial_rename_tables($old_type, $new_type) {
    + * @param $entity_type
    

    Missed param type.

  5. +++ b/serial.inc
    @@ -80,22 +83,24 @@ function _serial_rename_tables($old_type, $new_type) {
    + *   The name of the entity type that contains the field (e.g. content type)
    

    Missed dot at the end.

  6. +++ b/serial.inc
    @@ -128,8 +133,10 @@ function _serial_get_table_schema() {
    + * @param $entity_type
    

    Missed param type.

  7. +++ b/serial.inc
    @@ -128,8 +133,10 @@ function _serial_get_table_schema() {
    + *   Type of entity (e.g. node)
    

    Missed dot at the end.

  8. +++ b/serial.inc
    @@ -165,49 +172,75 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
    + * @param $entity_type
    

    Missed param type.

  9. +++ b/serial.inc
    @@ -165,49 +172,75 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
    + *   Type of entity (e.g. node)
    

    Missed dot at the end.

  10. +++ b/serial.inc
    @@ -165,49 +172,75 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
    +  $bundle_key = $entity_info['entity keys']['bundle'];
    

    Remove this line.

BR0kEN’s picture

Status: Needs review » Needs work
+++ b/serial.inc
@@ -165,49 +172,75 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
+      entity_save($entity_type, $entity);

Will not work without "Entity API" module (undefined function).

BR0kEN’s picture

+++ b/serial.inc
@@ -165,49 +172,75 @@ function _serial_generate_value($bundle, $field_name, $delete = TRUE) {
+  $bundle_key = $entity_info['entity keys']['bundle'];

Will not work with "comment" entity that have "node_type" value of "bundle" entity key and it's does not exist in schema.

BR0kEN’s picture

+++ b/serial.module
@@ -184,3 +183,57 @@ function serial_field_widget(&$form, &$form_state, $field, $instance, $items, $d
+          $pattern = pathauto_pattern_load_by_entity($entity_type, $data[$entity_type]->type);

$data[$entity_type]->type - will not work with "comment" entity that have "node_type" property instead of type.

BR0kEN’s picture

Assigned: lukus » BR0kEN
joachim’s picture

+++ b/serial.inc
@@ -187,33 +184,23 @@
+      $entity = entity_load_unchanged($entity_type, $entity->{$entity_info['entity keys']['id']});

You can use entity_extract_ids() here instead. That gets you the bundle too.

BR0kEN’s picture

FileSize
14.23 KB
1.81 KB
BR0kEN’s picture

Maybe anybody leave some comments?

ron_s’s picture

@BR0kEN, I haven't had a chance to test the update yet. Hoping that's something I can do in the next week or two, and will let you know if we identify any issues.

We have been running patch #56 for a while. The one issue we've recently noticed is every few weeks, there will be a non-unique serial ID. For example there will be an entity created with serial ID of 382, and then the very next entity will also have a serial ID of 382. Happens extremely rarely, but is certainly an issue that needs to be resolved. Seems like it might be a situation where the first entity is being edited while the second is created. Any thoughts? I haven't yet investigated it any further.

Thank you for your efforts.

colan’s picture

Status: Needs review » Needs work

Maybe we need to wrap that code in a transaction?

ron_s’s picture

@colan, I think so, would make sense.

BR0kEN’s picture

@colan, "that code"? Which?

colan’s picture

I guess whichever code that can't be running more than once at the same time that causes the same ID to show up multiple times. I haven't looked at this in a while, so not sure. Just making a suggestion for whomever's working on this.

It could be outside the scope this issue. Maybe we need to create another ticket for it? Does it happen without this patch as well? These two things could be unrelated. So we need to solve the race condition, but maybe not in this ticket. If that's the case, we can move ahead with this one if there are no other problems.

@ron_s: Were you having the multiple ID problem before this patch?

Can anyone else (dis)confirm?

ron_s’s picture

@colan, we're exclusively using the Serial module with entities. Therefore our only baseline is use of Serial with this patch.

You could be right that the race condition is outside the scope of this issue. Unfortunately I don't have data to confirm or deny. We'd need someone else to respond to know for sure.

colan’s picture

Status: Needs work » Needs review

@ron_s: I just found #2455677: Getting Duplicate numbers and gaps in the sequence order so I'm assuming it's not related to this. Please head over there for that.

Let's wait for another positive review or two here before committing.

BR0kEN’s picture

  1. Added SimpleTest for "node" & "comment" entities
  2. Added support of "db_transaction()" for insert operations
  3. Added experimental table naming, to meet limit in 64 symbols for table names

Let's try this one. But, want to say that these improvements must not be committed in this thread.

BR0kEN’s picture

Just code style improvements.

BR0kEN’s picture

FileSize
27.01 KB

Just merge with upstream. Unable to provide an interdiff, because file is reconstructed according to #2144389: Doesn't work with node_clone.

  • BR0kEN committed 4f73c53 on 7.x-1.x
    Issue #1333032 by BR0kEN, lukus, ron_s, tien.xuan.vo: Add generic entity...
MustangGB’s picture

Status: Needs review » Fixed

So this is fixed now right?

ron_s’s picture

Yes.

colan’s picture

@BR0kEN: How do you feel about cutting a new release? If we get this one published, we can stop talking about it on the project page.

BR0kEN’s picture

@colan, I guess we should update the project page. As for me - everything is fine.

colan’s picture

Version: 7.x-1.x-dev » 7.x-1.6

Looks like you already updated the project page. I just cut 7.x-1.6 to match.

kaizerking’s picture

lukus @27 , i am sorry for being away for too long. Do you still think i should open each point as separate feature reuest , please let me know i would do so

MustangGB’s picture

Status: Fixed » Closed (fixed)

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