Hello.

I found bug in process of working bean with EntityAPI. It will be happened when bean have numeric delta.
Delta will be set from title, when bean block is saving.

Description:

For example - you want to delete bean block with "delta" = 5 and real "bid" of this block is 3.

Part of callstack:
...
// Delete selected bean.
BeanEntityAPIController->delete();
// Load bean before delete.
EntityAPIControllerExportable->load();

in this function used snippet of code:

    // Only do something if loaded by names.
    if (!$ids || $this->nameKey == $this->idKey || is_numeric(reset($ids))) {
      return parent::load($ids, $conditions);
    }

In case with beans variable $ids contains "delta" of bean. In our example it's "5". 5 is numeric and in this case, entityAPI will load bean via "idKey" (bid). And when I will try to remove bean with delta = 5 and real bid = 3, entityAPI will load bean with bid = 5. If this bean is exists, it will be removed. If not, then no actions and I cannot remove my bean.

Comments

eugene.ilyin’s picture

StatusFileSize
new543 bytes
    // Only do something if loaded by names.
    if (!$ids || $this->nameKey == $this->idKey || is_numeric(reset($ids))) {
      return parent::load($ids, $conditions);
    }

This case also used for menu and I think that easy way to solve this problem - add prefix for delta.
This patch will solve this problem.

If you will apply my patch, please specify me as author of commit. Also I can attach script for repair deltas for old beans, but I don't know the best way for run this script. It can be hook_update, but it use batch. Hook_update can be runned from drush and I cannot make batch, working from drush and located in .install file (not as drush command).

Thank you for your module.

eugene.ilyin’s picture

Status: Active » Needs review
eugene.ilyin’s picture

very long testing : -\

eugene.ilyin’s picture

StatusFileSize
new848 bytes

Little fix in this patch

saltednut’s picture

Status: Needs review » Needs work

The tests are broken, I think. This is a great catch. I could easily delete bean with bid 1 simply by deleting bean with bid 2 but having a delta of 1. Wow. :(

Nice work here.

Some things needed to be added to get this in.

bb as a prefix introduces a whole new problem for the bean namespace. The prefix can just be "bean"

Also, I think we only need to append the prefix when the delta provided is purely numeric.

I added an if statement to take care of these problems and fixed the name but I'm giving eugene.ilyin commit credit for doing the majority of the work here.

Note that this still doesn't fix the heart of the issue - which is a flaw to Entity API. But we can't have it messing up Bean's UX so we'll account for it.

You can still delete beans errantly by providing bid in the url for say, /block/1/delete -- but 90% won't try to delete Beans this way.

http://drupalcode.org/project/bean.git/blobdiff/3227c5a47e74092406464ae3...

saltednut’s picture

Status: Needs work » Fixed

Just realized I errantly created a reversion here where the max_length wasn't always trimming correctly. Fixed in followup.

Status: Fixed » Closed (fixed)

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