Updated: Comment #143

Problem/Motivation

Sites on PHP 5.4 will see notices when editing nodes with multiple taxonomy terms:

Notice: Array to string conversion in function DrupalDefaultEntityController->cacheGet() (string 368 in file includes/entity.inc).

Steps to reproduce, from redsd in #122:

The error will surface when following these steps:

  1. Create a new node.
  2. Add multiple tags
  3. Save node
  4. Create a second node
  5. add multiple tags, but make sure you use atleast one tag that allready exists.
  6. Save node
  7. The following error will appear one or multiple times: Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364 of \drupal_standard\includes\entity.inc).

Or when

  1. Choose node with multiple tags
  2. Delete node
  3. On confirmpage the error will surface, one error for each term.

Or when

  1. Choose node with multiple tags
  2. edit node
  3. Don't change anything
  4. Save node
  5. The following error will appear one or multiple times: Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364 of \drupal_standard\includes\entity.inc).

There is no error when only new tags are entered and when the node is actaully deleted (after the confirm).

Proposed resolution

The patch in #143 seems to fix it. Note that this issue only affects 7.x, because the offending code has been removed from 8.x.

Remaining tasks

Tests need review.

Original report by Georgique

PHP 5.4
Got notice when saving node:
Notice: Array to string conversion in function DrupalDefaultEntityController->cacheGet() (string 368 in file /home/george/www/inti.kz.dev/includes/entity.inc).
Looked into code and don't understand why I see it. So can't make patch myself.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Georgique’s picture

Title: Error in DrupalDefaultEntityController->cacheGet() when saving node » Using array_diff_assoc() for multilevel arrays in DrupalDefaultEntityController->cacheGet()
Priority: Normal » Major
Status: Active » Needs review
FileSize
591 bytes

I found it.
Function array_diff_assoc() converts all elements of input arrays to string. But $entity_values can and contains elements which are arrays too.
So solution is serialize each element of array before comparing. Patch is attached.

bleen’s picture

Version: 7.12 » 8.x-dev

I havent checked, bu this looks like something that would have to be fixed in D8 first and then backported

Berdir’s picture

Issue tags: +Needs tests

Pretty sure this needs a test then... not sure I fully understand it.

tim.plunkett’s picture

Priority: Major » Normal

I don't think a notice is a major, bumping down.

barraponto’s picture

Can we get a wrapper? This is starting to pop up on other modules like Views, Features and even core itself.

Georgique’s picture

There is the same problem with using array_diff() also. I've checked. It should be repaired in many places.

rogical’s picture

got same issue in D7.14

tim.plunkett’s picture

Issue tags: +Needs backport to D7

.

tim.plunkett’s picture

Status: Needs review » Needs work

CNW for tests.

Anonymous’s picture

Status: Needs work » Reviewed & tested by the community

The patch from #1 works and is needed in Drupal 7.14.

Please move forward.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

Core patches need tests before being considered finished. See http://drupal.org/core-gates#testing

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
1.4 KB

Actually, I think using array_diff_assoc() was wrong in the first place, because that means we don't support array values for $conditions, like we do (by chance) when loading entities from the database.

Attaching patches for 8.x and 7.x that should fix this properly. I can provide some tests on Friday if we agree on the approach.

iSoLate’s picture

Reviewed the patch for Drupal 7, looks great. I tested it and it works fine.
I did not have the time to test for Drupal 8.

tracker2k’s picture

Patch #12 works perfectly for me on Drupal 7

Xano’s picture

attiks’s picture

Status: Needs review » Needs work

@amateescu can you write a test for Drupal 8, so we are sure this works?

Pere Orga’s picture

Same problem here on a fresh install of 7.15

Patch #12 seems to solve it.

alecliew’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs backport to D7

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

The last submitted patch, 1525176-fix_entity_conditions.patch, failed testing.

naveko’s picture

FileSize
1.45 KB

This patch works for Entity 7.x-1.0-rc3. This is the first time I post a patch, so hopefully I haven't done anything wrong.

Georgique’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, entity-1525176-#20.patch, failed testing.

slippast’s picture

#1 and #12 worked for me with Drupal 7.16 (not a fresh install).

Please note that this is a serious issue, more than just a PHP warning. I was unable to actually update my nodes. Following the long string of PHP warnings Drupal had a notification saying that the changes were not saved.

This problem appeared at some point on a MAMP sandbox server running PHP 5.4.4. I'm not sure when exactly because most of the node creation and updating is automated.

Thanks to amateescu.

salomos’s picture

Version: 8.x-dev » 7.x-dev
Category: bug » support
Priority: Normal » Major

Can anyone tell me what the status of this problem is? I downloaded Crommerce and have the very same issues described above. I only found one download of Commerce 7 that would actually work on my WAMP but it has this error all over the place. If I can't find the solution, I will have to go back to Ubercart. I didn't experience it until I downloaded the Commerce distribution. I am a newbie and not sure how to find out more information or a solution. Thanks everyone.

Pere Orga’s picture

Version: 7.x-dev » 8.x-dev
Category: support » bug
Priority: Major » Normal

Restoring version, category and priority.

Mark_L6n’s picture

FYI, http://drupal.org/node/1829260#comment-6677906 points out a PHP problem with array_diff_assoc.
I have this issue in 7.16. What is the consensus: is it safe to ignore these warning messages? If not, perhaps this should be changed back to 7.x and 'major'.

attiks’s picture

#26 it should be fixed for Drupal 8 first

Mark_L6n’s picture

I don't understand why it should be fixed for D8 first, if it is causing problems for D7 sites currently in use and is an actual problem (not just faulty warning messages) . Please don't misunderstand--I'm not being confrontational, but as an outsider to the process, it would seem that the current sites being used should have priority over the future dev project for non-trivial bugs.

barraponto’s picture

@mark123 we fix d8 first in order to prevent the error from showing up in the next release. Once it's fixed, we backport the patch so d7 (and d6) get the fix as well.

salomos’s picture

Does anyone know when this fix might be available? I am just looking for a rough estimate (ballpark figure). I am new to Drupal and I am not really sure how long NORMAL fixes seem to take. Should I look at another solution or hang out for this fix. I am not going live for at least 3 to 4 more weeks. Thanks everyone for helping a newbie!

paladin356’s picture

Version: 8.x-dev » 7.17

Changing line from the patch in post #1 in entity.inc worked for me. I do not get those messages anymore. Thanks.

tim.plunkett’s picture

Version: 7.17 » 8.x-dev

Fixing version.

kenyan’s picture

Since 7.18 is current, I guess this issue moves to it too.

Peter Bowey’s picture

Patch #12 works fine on Drupal 7.18

amin0214’s picture

Status: Needs work » Needs review
kclarkson’s picture

So will this patch be added to the next Drupal 7.19 release ?

bleen’s picture

Nope

a) no patch gets committed until it passes all the tests (aka turns Green in the issue queue) and is marked as Reviewd & Tested by the Community.
b) bugs are always fixed in the latest branch (in this case 8.x) first so that we dont forget to put the fix in there and thus re-introduce the bug

Neither of those two things have happened yet. If you want to help move this issue along so it can get into D7, then the next step would be to create a patch against Drupal8 that passes all the tests.

Mark_L6n’s picture

Given that:

  1. This bug exists for D8 & D7
  2. The bug needs to be fixed first for D8, but there are a large number of D8-only issues as well
  3. This is a major/critical bug for D7

It seems appropriate to raise the priority of this issue to at least 'major', and perhaps 'critical'. I won't make the change myself, but leave it to those more involved with the solution to this issue to decide.

pfrenssen’s picture

Tagging.

pfrenssen’s picture

David_Rothstein’s picture

If this is preventing nodes from being saved (and if that is reproducible with Drupal core alone) this should be moved to major or probably critical. Nothing to do with Drupal 7 vs. Drupal 8 most likely; a problem like that would be equally critical for Drupal 8 too :)

However, it's not clear to me from reading this issue whether there are actually a reproducible set of steps (starting from a fresh installation of Drupal core) that result in the node not being saved at all...

guney’s picture

#1: cache_get.patch queued for re-testing.

Paul B’s picture

Version: 8.x-dev » 7.21

It looks like the buggy code was removed from Drupal 8 in August (see #1184272).

kclarkson’s picture

does this mean we can get it for 7 ?

Alan D.’s picture

Version: 7.21 » 7.x-dev
Status: Needs review » Needs work
Issue tags: -Needs backport to D7

Yes, this code was removed in #1184272-81: Remove deprecated $conditions support from entity controller

-      foreach ($entities as $entity) {
-        $entity_values = (array) $entity;
-        if (array_diff_assoc($conditions, $entity_values)) {
-          unset($entities[$entity->{$this->idKey}]);
-        }
-      }

Needs work as the patch doesn't apply and also from #11 this needs tests

David_Rothstein’s picture

David_Rothstein’s picture

Issue tags: -Needs backport to D7

.

pmol123’s picture

Is there an estimated time for this fix in D7 (7-21+)?

Is the Dev version of D7 fixed ?

I would gladly move on to D8, but there are not enough modules ported to D8 yet.

Patrick

niudongwei’s picture

#12 1525176-fix_entity_conditions-D7-do-not-test.patch

this works on 7.21, Thanks!

drupalion’s picture

Thanks. #12 1525176-fix_entity_conditions-D7-do-not-test.patch works on drupal 7.22

pmol123’s picture

OK thank you.

Patch applied to production version of http://www.ExpertWitness.com

I will report back in a few days with feed back.

Patrick

mellamoanton’s picture

FileSize
36.3 KB

Hi!

I'm having this issue. It happens since I use "tags". My problem is that I can't apply the #12 1525176-fix_entity_conditions-D7-do-not-test.patch
I'm using Drupal 7.21 on localhost with MAMP.

I attach the screenshot.

Thanks in advance!

barraponto’s picture

@mellamoanton #50 says it works on Drupal 7.22, did you try upgrading then applying?

mellamoanton’s picture

My apologies, I don't know why I was trying apply patch on Entity module, not Drupal core. Anyway, I could not apply the patch correctly –I don't know why–, so I did it manually –were a few lines–. Now, everything is perfect.

Thanks!

NaX’s picture

#12 works for me on 7.22.

Enric Climent’s picture

#12 1525176-fix_entity_conditions-D7-do-not-test.patch works for me on Drupal 7.22 with no fresh install (is a blog with a lot of modules preactivated and configured). Thanks.

So, are this patch will be included in next versión of Drupal 7.x (for example 7.23) or in Drupal 7.x-dev?

Sorry my english ;)

rballou’s picture

So to clarify, what are we waiting on getting this in Drupal 7? I can try to help out if possible.

pmol123’s picture

Seems to be all good here.

I applied the patch #68 at issue:

==> http://drupal.org/node/1850798

this morning on my production drupal site on

==> http://www.ExpertWitness.com

Patrick

Berdir’s picture

As said before, we are waiting for a test. Without a test that exposes this bug and proves that the patch fixes it and will ensure that it won't get broken again, this will not get in.

drupov’s picture

For me the error went away with 1525176-fix_entity_conditions-D7-do-not-test.patch from #12 too.

martink’s picture

I tried applying the one-liner patch proposed in #1 (in line 369 on D7.22) and the error has gone away for the moment. I will upgrade to the next version when the issue is fixed definitively!

lucascaro’s picture

re-rolling the patch so it get's tested for D7..

edit: saved too quickly, see next comment.

lucascaro’s picture

Status: Needs work » Needs review
FileSize
1.21 KB

re-rolling the patch so it get's tested for D7..

sokrplare’s picture

#63 worked great for me!

andrea.cavattoni’s picture

also replace the "array_diff_assoc" with 'array_diff_assoc_recursive' and copy this function in a module or in the core:

function array_diff_assoc_recursive($array1, $array2) {
  foreach ($array1 as $key => $value) {
    if (is_array($value)) {
      if (!isset($array2[$key])) {
        $difference[$key] = $value;
      } elseif (!is_array($array2[$key])) {
        $difference[$key] = $value;
      } else {
        $new_diff = array_diff_assoc_recursive($value, $array2[$key]);
        if (count($new_diff) > 0) {
          $difference[$key] = $value;
        }
      }
    } elseif (!array_key_exists($key, $array2) || $array2[$key] != $value) {
      $difference[$key] = $value;
    }
  }
  return !isset($difference) ? array() : $difference;
}

damned php 5.4!

dgeo’s picture

This last patch works for us too.
Please commit this to 7.x

delta’s picture

Status: Needs review » Reviewed & tested by the community

Works for us too, on two fresh project, we have the same problem.
#63 fix it, and i didn't have any regression after the patch.

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
Berdir’s picture

Lost my comment. This still needs a test, otherwise it can't be commited.

lucascaro’s picture

Status: Needs review » Needs work

setting to needs work for the tests

prathK’s picture

Similar Notices I observed in commons 3.0 while editing events and it got fixed with #63.

Thanks !!!! :)

pfrenssen’s picture

Wouldn't this be solvable with the drupal_array_diff_assoc_recursive() function that is being added in #1850798: Add a recursive version of array_diff_assoc()? That issue has tests, has already been committed to D8, and has a backport for D7 in RTBC state.

edster1’s picture

#63 worked great on 7.22 for me!

lucascaro’s picture

@pfrenssen I think you have a point, as soon as that patch is backported, we need to create a patch here using drupal_array_diff_assoc_recursive() instead.

Yazzbe’s picture

#63 works for me

xcafebabe’s picture

#63 worked for me as well, thanks !

David_Rothstein’s picture

FYI: I just committed #1850798: Add a recursive version of array_diff_assoc() to Drupal 7 (in case that helps move this issue forward, as several of us mentioned above).

andypost’s picture

is not it a release blocker?

Mark_L6n’s picture

Issue tags: +PHP 5.4

Adding issue tag 'PHP 5.4'.

alesr’s picture

Issue tags: -PHP 5.4

Where are tests for this issue?
It would be good if this patch is commited to the next D7 release?

knowledges33ker’s picture

I also need this. I've patched contrib modules before but never patched drupal core. Can anyone tell me where I put the patch file (in what directory?) to have the patch applied correctly?

bleen’s picture

@knowledes33ker .. just like a patch for a contrib module should be applied relative to the root of the contrib module, patches for core should be applied relative to the root of core. There is really good information at http://drupal.org/patch on how to do this. Happy patching

JamesOakley’s picture

Issue tags: +PHP 5.4

@alesr (#80) - did you clear the PHP 5.4 tag for a reason? Given PHP 5.3 is EOL, and 5.4 is now considered by PHP to be an "old stable" release, ensuring Drupal is fully PHP 5.4-compliant is quite important. I appreciate that tag bloat is a bad thing, but tagging issues that directly relate to PHP 5.4 compliance is either harmless or helpful, IMO.

alesr’s picture

Issue tags: -PHP 5.4

@JamesOakley, no, the tag was removed when I replied.
Hope it won't go away this time again.

EDIT: And it did.. Looks like commenting bug with tags?

tstoeckler’s picture

Issue tags: +PHP 5.4

Yeah, that can happen sometimes. Drupal.org strangeness...

handrywahyudi’s picture

Status: Needs work » Needs review

#1: cache_get.patch queued for re-testing.

David_Rothstein’s picture

Just to clarify, I don't think this issue is really related to PHP 5.4 compliance - see discussion in #1850798: Add a recursive version of array_diff_assoc(). If the code is incorrect, it's incorrect for other versions of PHP too (the only difference is that PHP 5.4 is noisier about alerting you to the fact that your code is probably not doing what you think it is).

elgwiedo’s picture

#63 works for me as well.

marcofloriano’s picture

Version: 7.x-dev » 7.22

Getting this error while trying to update a node through the edit widget at front page. Not sure what i´m suppose to do, apply the patch, turn off warning report or wait for the tests and update.

gowrann’s picture

#63 works for me too on 7.22

apaderno’s picture

Version: 7.22 » 7.x-dev
maximpodorov’s picture

Drupal includes drupal_array_diff_assoc_recursive() in 7.23, so it's time to fix the issue using this function.

pslcbs’s picture

#63 worked for me on Drupal 7.23 and php 5.4

Thank you!!

Peter Bowey’s picture

Patch #63 worked for me via Drupal 7.23 and php 5.5.1

Thank you!!

flocondetoile’s picture

Patch #63 works fine on Drupal 7.23 and PHP 5.4.17
Thanks

star-szr’s picture

Status: Needs review » Needs work

Regardless of the fix, this still needs tests. Setting to needs work.

Edit: Providing clear steps to reproduce from a clean install of Drupal 7 in the issue summary would help in writing automated tests, see https://drupal.org/contributor-tasks/add-steps-to-reproduce.

star-szr’s picture

Adding Needs tag for steps to reproduce.

jorgemontoyab’s picture

#63 works fine. Drupal 7.23, PHP 5.4.9-4.

goldenboy’s picture

@maximpodorov, @cavax added patch which use drupal_array_diff_assoc_recursive instead of array_diff_assoc.

This patch need Drupal 7.23 at least.

SocialNicheGuru’s picture

is the patch in 99 instead of the one in 63? which supersedes which?

dlu’s picture

Status: Needs work » Needs review

So the testbot can have its fun.

Status: Needs review » Needs work

The last submitted patch, drupal7.entity-system.1525176-99.patch, failed testing.

dlu’s picture

In response to #100, I'd prefer #99 over #63 as it is simpler and reuses existing code. But for older version of D7 (if you can't update for some reason) then #63 could be an option.

dlu’s picture

Status: Needs work » Needs review
FileSize
560 bytes

Here's a reroll of #99. Let see if the bot likes it better.

dlu’s picture

Status: Needs review » Needs work

Since the patch passed and we still have issues – steps to reproduce and maybe tests, I'm changing this back to 'needs work.'

From #72, #74, and #77 above it sounds like the 'Needs tests' tag could be removed leaving only the steps to reproduce before RTBC, or do we want tests specific to this case?

star-szr’s picture

This bug should be possible to test so I think we should (ref. https://drupal.org/core-gates#testing). Having steps to reproduce the bug would help very much with writing tests and getting this issue resolved.

Berdir’s picture

This only fails on 5.4, so we can't yet write a test that visible fails on the testbot but it should be possible to confirm manually and be prepared when we will test on 5.4 as well.

Homotechsual’s picture

So as we can't test can we go for RTBC and get this into a release?

Makes sense to me that if we cannot test because the testbot doesn't test on PHP 5.4 then we should get the fix committed rather than waiting for testbot to work with PHP 5.4.

Confirmed #104 works on D7.23

Berdir’s picture

No, this still needs a test that proves that it fixes the bug on 5.4. That test can be executed manually to confirm it.

dlu’s picture

No we should still do the tests. I'm going to take a shot at writing them in the next week if I can. We also need steps to reproduce. Any chance you could contribute them?

Yazzbe’s picture

#104 worked for me on D7.23

pineiden’s picture

It's work to me, i had the same problem and i replace de 'if' line
thank you!

huzooka’s picture

Same situation here. After patching the notice message is gone.
7.23

flocondetoile’s picture

Me too. #104 worked on D7.23

GuGuss’s picture

Patching entity.inc with #104 fixed the notice on D7.23.

Road Kill’s picture

I had this issue when up loading an image and confirm that patching entity.inc with #104 fixed the notice on D7.23. :)

russellb’s picture

Hi, we're seeing this notice a lot on a live site, so I'm having a go at steps to reproduce:

PHP 5.4
1) Create new article
2) Add multiple terms to tags field
3) Save node

.. and I'm seeing the notice once if I put 2 terms in the tags field, the notice occurs 3 times if I put 3 terms in the tags field:

Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 369 of ... includes/entity.inc).

redsd’s picture

Got the same problem when trying to save multiple terms to a node field.
The nodes will save though so it's only the warning with no code breaking results.

I'm in no position to complain, but it's been over a year since a fix have been supplied, how can this still not be resolved?

dlu’s picture

I'm in no position to complain, but it's been over a year since a fix have been supplied, how can this still not be resolved?

Fixes get stalled when nobody steps up to finish the job. In this case we need a test (so that we know that the problem is fixed and stays fixed) along with the steps to reproduce the problem. On top of that the problem (probably) needs to be addressed in D8 as well – at the very least we need to know that the changes in D8 mean that this problem won't be there. When a problem exists in D8 we start fixing it there to ensure that the bug fix is in the most recent version, then we back port to older versions.

It looks like #117 may provide the steps to reproduce, so I've removed the tag. Now if somebody could write the tests we'd be moving.

russellb’s picture

Updating my proposed steps to reproduce here, I think you need pre-existing terms to get the notice on a first node save. The notices also occur on node delete so this may give us the simplest steps to reproduce:

PHP 5.4
1) Create new article
2) Add multiple terms to tags field
3) Save node
4) Delete node

I've not been able to test this on a fresh Drupal 7 install on PHP 5.4 yet.

russellb’s picture

I didn't mean to switch the issue tag there but maybe is best until steps to reproduce are verified on a clean install.

redsd’s picture

My test findings are for drupal 7.23 standard install using php 5.4.12:

The error will surface when following these steps:

  1. Create a new node.
  2. Add multiple tags
  3. Save node
  4. Create a second node
  5. add multiple tags, but make sure you use atleast one tag that allready exists.
  6. Save node
  7. The following error will appear one or multiple times: Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364 of \drupal_standard\includes\entity.inc).

Or when

  1. Choose node with multiple tags
  2. Delete node
  3. On confirmpage the error will surface, one error for each term.

Or when

  1. Choose node with multiple tags
  2. edit node
  3. Don't change anything
  4. Save node
  5. The following error will appear one or multiple times: Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364 of \drupal_standard\includes\entity.inc).

There is no error when only new tags are entered and when the node is actaully deleted (after the confirm).

dlu’s picture

Assigned: Unassigned » dlu

Redsd, thank you for the steps! I'm going to take a shot at writing the tests.

eth01’s picture

Version: 7.x-dev » 7.23

Drupal 7.23

I'm having the same error Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364 of ...\includes\entity.inc)

Offlein’s picture

Version: 7.23 » 7.x-dev
fraweg’s picture

Hello,

I have this issue after upgrading advanced Forum as you can see here:

https://drupal.org/node/2056005#comment-8010695

But #104 did not work for me... Any ideas ?

Best regards
Frank

letapjar’s picture

drupal 7.23 - Manually applied the patch from #104 nd it resolved my probelem of:
Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364

mari3.14’s picture

Working on a website using Drupal 7.23 when I encountered the same issue. Patch #104 worked for me.

Sofia-ED’s picture

Reporting the following comment as spam: https://drupal.org/node/1525176#comment-7421712

Hi! Thank you very much for the patch, but I've a question: How I install the patch? x) of the post 105

https://drupal.org/comment/7812787#comment-7812787

Thanks for the help ^^

cutmedia’s picture

+1 for #104 worked for me on D7.24, PHP 5.4

gowrann’s picture

+1 for #104 worked for me on D7.24, PHP 5.4.17

David_Rothstein’s picture

No, this still needs a test that proves that it fixes the bug on 5.4. That test can be executed manually to confirm it.

The functional bug here exists on all PHP versions (the only difference with PHP 5.4 is that it's noisier in terms of warning about it). So it should be possible to write a test for this that isn't dependent on PHP 5.4.

MXT’s picture

+1 #104 worked for me on D7.24, PHP 5.4.22

Thank you and please commit this!

NaX’s picture

drupal_array_diff_assoc_recursive() was committed with tests and now is recommend to be used over the built in function array_diff_assoc() #1850798: Add a recursive version of array_diff_assoc()

There have been more than a few posts here that show easily and clearly how to reproduce this issue.

Can somebody please explain what is holding up this being committed to D7. This patch is a simple swapping out of array_diff_assoc() with drupal_array_diff_assoc_recursive(). I don't see why this cant be marked RTBC.

Just my 2 cents.

lukio’s picture

+1 #134

Offlein’s picture

Sounds like it needs a test, right? No test, no commit?

NaX’s picture

The parent issue that introduced the new function has a test, cant we assume that test covers us. Are we now suppose to create a duplicate test testing the same thing. I don't really understand what needs to be tested here when there is already a test for the function we are using. Can somebody please explain, I think I am missing something.

drupal_array_diff_assoc_recursive test: http://drupalcode.org/project/drupal.git/blobdiff/3201287ba449a71ea01c21...

pslcbs’s picture

#104 worked for me on 7.24 and php 5.4.23

Thank you!!

plasmid_stha’s picture

i am using drupal 7.29... and i m new to drupal...which patch should i use (or it is trial and error practice)......

NaX’s picture

@plasmid_stha
As a rule I would always test the last patch submitted related to your version. So in your case patch found at comment #104. But its always good to read some of comments as they might point to a different patch that would be better for you and once you get some background to the issue you will maybe able to assist and provide feedback.

francescosciamanna’s picture

#104 did not work for me on Drupal 7.24 and php 5.2.17 :S Any idea more?

webchick’s picture

I ran into this issue while working on some Drupal.org stuff, so tagging. I've also updated the issue summary, since this issue is a bit of a mess.

I find it interesting that none of the existing tests seem to catch this, even on the PHP 5.4-specific testbot: https://qa.drupal.org/pifr/test/600308#tabset-tab-4

webchick’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
147.18 KB
18.18 KB
1.88 KB

Here, this test seems to catch it, as well as add something that's probably useful to be tested, given the surrounding tests. Since testbot won't fail a test-only patch, here's a before/after screenshot from my localhost running PHP 5.4.8:

Before the patch:
9 exceptions, all related to cacheGet in entity.inc

After the patch:
0 fails after the patch

webchick’s picture

Issue summary: View changes
webchick’s picture

Btw, thanks so much to redsd and russellb for the clear steps to reproduce! Never would've been able to write that test without them. :) Would be great to credit them on commit.

Status: Needs review » Needs work

The last submitted patch, 143: drupal7.entity-system.1525176-143.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review

LIES. :)

143: drupal7.entity-system.1525176-143.patch queued for re-testing.

pmichelazzo’s picture

The patch #143 works fine for me.

Thanks!

webchick’s picture

If that's a "reviewed & tested by the community" then please feel free to mark the issue as such. :) Seems like David is on a commit spree lately, so it'd be a good time to do so. However, someone should really look over those tests I added and make sure they look sane first.

Berdir’s picture

Tests look ok to me, whatever covers this. Wondering if it's worth to add a comment that explains we're also testing this, as they're fairly indirect.

Also not sure if the hardcoded assertText() message is more helpful than the default "$term was found", because usually you want to know *what* it didn't find.

maximpodorov’s picture

Status: Needs review » Reviewed & tested by the community

The patch solves the problem. Maybe it's time to commit it and make 7.25 fully PHP 5.4 compatible.

andypost’s picture

The functional bug here exists on all PHP versions (the only difference with PHP 5.4 is that it's noisier in terms of warning about it). So it should be possible to write a test for this that isn't dependent on PHP 5.4.

Now #132 addressed so David please include this fix in upcoming "New year release"

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
3.59 KB
4.14 KB

If the test only fails on PHP 5.4, then it's only testing the PHP notice, not the functional bug.

Although after looking at this for a bit, I found it pretty difficult to come up with a situation where the functional bug is actually triggered. Which makes me confused about #23 (and other reports above that this is a bug with actual consequences) as well as the fact that this issue has 150 followers... PHP notices by themselves aren't really that big of a deal, especially if they aren't triggered frequently.

Anyway, it is possible to test it, although the test is a bit contrived. This one should pass/fail on the testbot as appropriate. I left @webchick's test too, of course, since that looks good in terms of improving the general test coverage (and hers is probably forward-portable to Drupal 8, but mine isn't).

The last submitted patch, 153: drupal7.entity-system.1525176-153-TESTS-ONLY.patch, failed testing.

David_Rothstein’s picture

Hm, testbot messages are a bit confusing these days, but you can see above that the patch which was supposed to pass passed, and the patch which was supposed to fail failed.

maximpodorov’s picture

Status: Needs review » Reviewed & tested by the community

So we can mark it as RTBC again.

maximpodorov’s picture

@David_Rothstein, are there any issues preventing this to be committed?

webchick’s picture

I think the ire probably stems from the fact that if you find yourself in PHP 5.4 editing a node with multiple terms (for example, almost every single issue on a Drupal.org test site :D) the notices start to become really irritating. It seems like you get 1 notice per tag, so on some issues I was getting a screen-full of notices.

That's a much better/more direct test, thanks. :) Agreed with forward-porting test coverage to 8.x once this is fixed in 7.x.

webchick’s picture

Oh, and I think #23 is a red herring, or at least I wasn't able to reproduce that on either PHP 5.3 nor PHP 5.4 in writing the test that tests that exact scenario. May have been some other module's stuff that conflicted in that case.

mpark’s picture

Why do not embedded in the new patch?

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

Array conditions only work by accident, they were never supposed to be supported.

They only work because DBTNG automatically use IN when you pass an array to a condition like:

$query->condition("column", $value);

... will be column = 'value' if value is a scalar or column IN ('value1', 'value2') if value is an array.

We discussed removing support for this, but I guess now it's too late :(

So now, this is probably not what we want:

-        if (array_diff_assoc($conditions, $entity_values)) {
+        if (drupal_array_diff_assoc_recursive($conditions, $entity_values)) {
           unset($entities[$entity->{$this->idKey}]);
         }

Suppose that $conditions contains array("uid" => array(1, 2, 3)), the drupal_array_diff_assoc_recursive() is not going to do the right thing.

The easiest way to test that would be to try to load by id and conditions an entity that is already in the cache, like:

entity_load('node', array($id), array('uid' => array(1, 2, 3)));
mpark’s picture

nice

miromarchi’s picture

Issue summary: View changes

Minor typo: The patch in #143 (not 142) seems to fix it.

firesidelibrarian’s picture

So, which patch should I use, #143 or #153?
Thanks for working on this!

AlterEgoEast’s picture

F1 please
how do I use patch # 143
Thanks in advance!!

AlterEgoEast’s picture

F1 please
how do I use patch # 143
Thanks in advance!!

alesr’s picture

@firesidelibrarian:
#153 patch has entity CRUD tests added.
That's the difference between patch #153 and #143.
So try with #153 first and if you experience issues, go for #143 and report.

@AlterEgoEast:
Please check this out https://drupal.org/patch/apply

busla’s picture

Is there any way to know if this was commited to 7.26?

mikeytown2’s picture

@busla
Check the release notes: https://drupal.org/drupal-7.26-release-notes

If you want more in depth details check the git shortlog: http://drupalcode.org/project/drupal.git/shortlog/refs/heads/7.x As you can see only 2 issues where committed to 7.26:
#2164179: Add links to Extender documentation in Database API
https://drupal.org/SA-CORE-2014-001

Also there will usually be a message in the thread stating that a patch was committed; looking over this issue I do not see any message.

Given all of these things I can safely say that this issue has not been fixed in 7.26 :)

slipperysky’s picture

PHP 5.5.8 (cli)

Installed Drupal 7.26, admin_menu, adminimal_admin_menu, ctools, entity, libraries, panels, token, views...etc.

Saw the error. Found this thread. Ran: git apply....patch #153

No problem. Error stopped.

Edit: Sorry, those files surprized me. Didn't realize that they would all be automatically attached. I've been using Drupal for a while but am new to posting here.

AaronELBorg’s picture

Slipperysky,

Add a term reference field to a node and make it an autocomplete widget.

Create a new node of that type.

Edit that node and add some terms in the autocomplete field.

Still no error?

slipperysky’s picture

AaronELBorg:

OK, I added a second term reference to an article as an autocomplete widget, created a new article, edited article by adding some terms... still no error. Tried a few times.

I noticed the error originally when creating/editing an article and now I don't see it.

So, it appears as though #153 fixed it for me without any problem. I can try running through the whole process again later on a fresh install (before the patch) when I get another chance....

drupov’s picture

Hi,

patch from #153 applies and solves the issue for me with drupal-7.26 also.

miromarchi’s picture

patch in #153 works for me as well. I've tested it in two different websites, in two slight different environments.
1) Ubuntu 13.04, drupal 7.23
2) Debian 7.2, drupal 7.26
Both now are smooth!
thanks

MXT’s picture

Follow up for my #133:

I've tested patch #153 in another environment and all works fine.

(D7.26, PHP 5.4.23)

Please commit this!

klausi’s picture

+++ b/modules/simpletest/tests/entity_crud.test
@@ -0,0 +1,45 @@
+    // Create a few nodes. One of them is given an edge-case title of "Array",
+    // because loading entities by an array of conditions is subject to PHP
+    // array-to-string conversion issues and we want to test those.
+    $node_1 = $this->drupalCreateNode(array('title' => 'Array'));
+    $node_2 = $this->drupalCreateNode(array('title' => 'Node 2'));
+    $node_3 = $this->drupalCreateNode(array('title' => 'Node 3'));
+
+    // Check that the first node can be loaded by title.
+    $nodes_loaded = entity_load('node', FALSE, array('title' => 'Array'));
+    ksort($nodes_loaded);
+    $this->assertEqual(array_keys($nodes_loaded), array($node_1->nid));
+
+    // Check that the second and third nodes can be loaded by title using an
+    // array of conditions, and that the first node is not loaded from the
+    // cache along with them.
+    $nodes_loaded = entity_load('node', FALSE, array('title' => array('Node 2', 'Node 3')));
+    ksort($nodes_loaded);
+    $this->assertEqual(array_keys($nodes_loaded), array($node_2->nid, $node_3->nid));

I think this test case is incomplete. The last entity_load() fetches the node from the DB, where the nested array condition works. But we need to to test cacheGet() so if you repeat the last entity_load() you will see different results.

Overall I think this notice is a legitimate error report that you have some client code that invokes entity_load() with nested array conditions, which should never be done. Use EntityFieldQuery instead.

We could try to use is_array() on each condition value and then iterate in a loop in cacheGet(), but that feels like re-inventing EFQ.

Are there invocations of entity_load() in core that do this?

ladybug_3777’s picture

I can also confirm on my install of Drupal 7.26 the patch in 153 works

arosboro’s picture

Can confirm, #153 works with latest Drupal 7.26

steveoriol’s picture

Ok for me too, #153 with D7.26 and PHP5.4.4-14
Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet()

jdexter’s picture

Hey @AlterEgoEast .. Been getting chummy with manual live site patching again myself - amazing what you forget after enough of a rest.. :)-

Usually, you can work out how a given patch is trying to apply by looking at the patch for file path its trying to use.

In this case, this is a multi-file patch spanning several subdirectories .. So first thing is to login and go to your site root (ie var/www/public_html/epicdrupisite/) and download the file from the command line:

wget http://drupal.org/files/issues/drupal7.entity-system.1525176-153.patch

If you copied the link - you will likely have to edit 'https' and stick with http as above ..

Saving locally and ftp'ing it in is kinda pointless because you will need ssh to actually apply the patch, but if wget is disabled - have a care to save the patch cut 'n paste in an absolutely known clean plain text. I use textpad - allows a several plain text options.

In either case, you should end up with a .patch file in the root directory at the top of your drup install ..

Next trick is help the patch find the right paths for the files - so - you issue a -p option on the command line - in this case:

patch -p1 < drupal7.entity-system.1525176-153.patch

.. there are several -p options, but in this case - you are telling the patch program that this particular patch file is relatively targeting file locations below the patch file and to strip the leading '/' thus:

patching file includes/entity.inc
patching file modules/simpletest/simpletest.info
patching file modules/simpletest/tests/entity_crud.test
patching file modules/taxonomy/taxonomy.test

.. above is what it should look like after running the command line noted for this patch ..

The primary variations you see are local patches - so no -p .. just

patch < drupal7.some.patch.1525176-153.patch

.. which would patch a file in the same directory as the patch .. and ..

patch -p0 < drupal7.some.system.1525176-153.patch

Which would set the path from drive root for a file like this:

patching file /etc/passwd
patching file /etc/group

.. Have a care when applying patches to pay attention to the date .. A lot of patches are absolutely unique to a given point in time and if your application is updated after the patch was issued, you may need to knit it in manually ..

NEVER use -f (force) option .. Its almost like they left it in there as some sort of embittered gag on the Mac/MicroGoat users out there ..

Cheers - as always - your mileage may differ ..

Colin @ PCMarket’s picture

Patch from #153 works for me too on Drupal core 7.26

klausi’s picture

Status: Needs work » Reviewed & tested by the community

I had another look at this today. Turns out that the proposed patch by David Rothstein works as is because the cache is simply bypassed with multiple values per property. The drupal_array_diff_assoc_recursive() will weed out all cached entities, because a simple title string "Node 1" will always be different to array('Node 1', Node 2'). In this case ->cacheGet() will always return an empty array, which means the ->load() method will fallback to the database in order to retrieve the entities.

So this is a minor performance problem for people using multiple value conditions with entity loading, because the static entity cache is always bypassed. But that performance problem has existed since Drupal 7.0 and we are merely fixing a bug with the special "Array" value here. So this indeed looks like the least invasive fix for Drupal 7 at this point.

@Damien Tournoud: I could not reproduce a problem with your node UID scenario, could you provide a test case? Otherwise I would say this should go back to RTBC.

mariusz.slonina’s picture

Patch #156 works with Drupal7.26 (at a bunch of modules)

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

Please see #161 for the complete explanation. What will happen is that we are not returning from the cache entities that we should be returning from the cache.

klausi’s picture

Status: Needs work » Needs review

Yes, I read your explanation. Bypassing the cache is not a problem in itself, since the load() method will fallback to loading from the database. Not ideal, but enough to fix the PHP notice and weird behavior with special "Array" values. See #182 for the complete explanation.

Damien Tournoud’s picture

Status: Needs review » Needs work

I do agree this is not a big problem, but it makes the logic of the patch incorrect, and as a consequence "Needs work".

Nick_vh’s picture

Sorry to spoil the fun but it still fails for me:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '))' at line 2: SELECT base.tid AS tid, base.vid AS vid, base.name AS name, base.description AS description, base.format AS format, base.weight AS weight, v.machine_name AS vocabulary_machine_name FROM {taxonomy_term_data} base INNER JOIN {taxonomy_vocabulary} v ON base.vid = v.vid WHERE (base.name LIKE :db_condition_placeholder_0 ESCAPE '\\') AND (base.vid IN ()) ; Array ( [:db_condition_placeholder_0] => Normal ) in DrupalDefaultEntityController->load() (line 191 of .../docroot/includes/entity.inc)

To add some context, I first got the bug mentioned in this issue and what follows is an accurate description of what happened here: https://drupal.org/node/1228076. It might be unrelated but still

To replicate:

Add two fields from the same taxonomy to the content type. Make both of them autotagging and part of the same taxonomy vocabulary. The second one won't be able to fill in the terms from the vocabulary anymore

Update: Removing the field and re-adding the field after encountering the bug seemed to fix it. Ignore this for now as it might be unrelated and a glitch of the matrix

klausi’s picture

Status: Needs work » Needs review
FileSize
4.96 KB

klausi opened a new pull request for this issue.

klausi’s picture

Implemented proper array condition handling in cache get as proposed by Damien, interdiff is here: https://github.com/klausi/drupal/commit/b8e7cee7efc5dae87e4211206720be84...

Damien Tournoud’s picture

From visual review, the logic looks sound. I sketched how to properly test this in #161: try loading an entity normally, then with an array-form condition, and assert that you get the same object.

drupov’s picture

#188 works for me on drupal 7.26.

Thanks!

klausi’s picture

FileSize
5.3 KB

Added a test case to check that the nodes loaded are indeed identical to ones cached before. Also tested it with the original drupal_array_merge_recursive() approach and the test case now fails as expected. Interdiff: https://github.com/klausi/drupal/commit/4ff957c41e28bece06385c329999e45f...

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Yes. This looks great now, thanks @klausi!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 192: 1525176.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review

192: 1525176.patch queued for re-testing.

mvwensen’s picture

Confirming that patch #192 works on Drupal 7.27

NaX’s picture

Status: Needs review » Reviewed & tested by the community
pslcbs’s picture

#192 worked for me on Drupal 7.27

arosboro’s picture

patch 192 had some issues with patching file modules/simpletest/tests/entity_crud.test but after applying the patch to includes/entity.inc, the error messages disappeared. Drupal 7.27 here.

Boobaa’s picture

Patch in #192 applies cleanly to D7.27, and solves the problem for me.

Garrett Albright’s picture

It worked for me too. Let's get this baby in core!

lomars’s picture

Patch #192 has solved this problem for me with D7.27. No more error message.
But it was not so easy to understand how to apply it (first time) on the differents files manualy.
Is there an automated maner to do that ? Thanks by advance.
It would be nice to have in core !

Garrett Albright’s picture

lomars, see this page for more information on patches and how to apply them: https://drupal.org/patch/apply

Saoirse1916’s picture

Agree, #192 did the job for me as well on 7.27.

forestgardener’s picture

I can also confirm that applying the patch from #192 on drupal 7.27 successfully removed the error messages.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

David_Rothstein’s picture

Also created #2259501: Forward-port tests for taxonomy terms being retained after node edit to forward-port the taxonomy tests here, as discussed above.

  • Commit 55b9d54 on 7.x by David_Rothstein:
    Issue #1525176 by klausi, amateescu, David_Rothstein, dlu, goldenboy,...

Status: Fixed » Closed (fixed)

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

makokis’s picture

Is this issue corrected in the new version 7.28?

thanks

forestgardener’s picture

@makokis Yes, the changes in the patch are present in the files of 7.28