Problem/Motivation

The current implementation of the flags module integration with the rules module can lead to a fatal error under certain situtations. The source of the fatal error is a bad operand to the array_merge function.

The behavior of array_merge() was modified in PHP 5. Unlike PHP 4, array_merge() now only accepts parameters of type array. However, you can use typecasting to merge other types.

One pathway to this error situation is when the module_implements variable becomes incomplete, possibly due to a bad cache flush under heavy load. In these situations sites will stop working properly, but should still bootstrap completely enough to correct the corrupted cache. Under this type of condition, the flag module doesn't do adequate sanity checks leading to the fatal error and incomplete bootstrap. Additional information on cache poisoning can be found in another thread: https://www.drupal.org/node/1934192#comment-7363318

When it tries to load all of the defined flags, it is expecting that it will always get at least the default link types. The issue we are seeing is being caused by flag_get_link_types() returning an empty array. In the corrupted cache situation, the flag.flag.inc file may not get loaded so the hook with the default link types is not run. Since the default link types are loaded via the flag_link_type_info hook, and then modified by the flag_link_type_info alter hook, the Flag module could also get any combination of results due to other poorly written modules. As a result, the $flag->get_link_type() method can return something other than an array (ie. NULL).

Specifically, when the $flag->options() method is called it does the following:

<?php
    // Merge in options from the current link type.
    $link_type = $this->get_link_type();
    $options = array_merge($options, $link_type['options']);
?>

Having a rule defined is not a requirement for this bug to appear, but having a flag defined is. The Flag module tries to interrate over all defined flags to expose them as events to Rules. The problem is simply assuming that the values being assigned both exist and are an array, which, while it's supposed to exist and supposed to be an array, can not simply be expected 100% of the time in a contributed, alterable ecosystem.

Proposed resolution

The code is already typecasting the $options variable.

$flag_options = (array) $flag->options();

Why not the same for the flag options?

$link_type = (array) $this->get_link_type();

That at least guarantees that when you do a += or array_merge, the values are empty arrays. The argument really isn't that they should never be empty, the argument is handling it better than a fatal error that kills the bootstrap process.

The fact is that in certain edge cases, the options are NULL or empty, which does not play well with += assignment of another type. Typecasting just ensures your data type is what you expect.

Remaining tasks

Once something like a simple typecasting casting is added to the code, nothing else needs to happen. All functionality will remain the same. It just removes a potential fatal error condition.

User interface changes

None.

API changes

None.

Original report by vinoth.3v

I am getting Fatal error: Unsupported operand types in flag/includes/flag/flag_flag.inc on line 130

Drupal 7.20
Flag 7.x-3.x-dev and 7.x-3.0-alpha4

any Idea?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Status: Active » Postponed (maintainer needs more info)

Are you using any modules that extend Flag with new types?

All of Flag's flag classes return an array for options(), so I don't see how this warning could come up.

When do you see the error?

> Flag 7.x-3.x-dev and 7.x-3.0-alpha4

Which is it?

vinoth.3v’s picture

Priority: Critical » Major
Status: Postponed (maintainer needs more info) » Needs review

error on both Flag 7.x-3.x-dev and 7.x-3.0-alpha4, but on different lines.

after debugging this module, the $flag is flag_broken Object! But it is not with the external module I hope. It is just a node type.

flag_broken Object
(
    [fid] => 3
    [entity_type] => node
    [name] => favorites
    [title] => Like
    [global] => 0
    [types] => Array
        (
        )
    [roles] => Array
        (
            [flag] => Array
                (
                )

            [unflag] => Array
                (
                )

        )

    [errors] => Array
        (
        )
)

and $flag->options() gives NULL;

and, It works after modifying those lines. I don't know why, in some rare situations flag object is broken.

    $options = (array) unserialize($row->options);
    $flag_options = $flag->options();
    if (!empty($flag_options)) {
      $options += $flag_options;
    }
joachim’s picture

Priority: Major » Normal
Status: Needs review » Postponed (maintainer needs more info)

Can you debug to see why your flag is getting set to broken?

It may be because you've disabled some modules.

vinoth.3v’s picture

Sorry, since it given me various errors I just deleted all old flag objects. (no problem, it is localhost)
but I was unable to create a new flag. There were No Flag type on create page. strange!

cleared the cache many times. no luck.

But it started working fine again automatically after some time. still not sure why.

joachim’s picture

Status: Postponed (maintainer needs more info) » Closed (cannot reproduce)

Very weird. Reopen if it happens again!

drupalion’s picture

HI. i have a same problem in drupal 7.22 and flag 7.3-dev. resolved problem with #2 solution.

joachim’s picture

I don't actually understand what the fix in #2 is meant to be. It would help if people posted proper patches!

Aciid’s picture

FileSize
494 bytes

Hey joachim, here's the patch that fix #2 proposes and is currently working, thanks vinoth.3v by the way. I'm not working with git on my desktop so i just used WinMerge, the result should be obvious enough should the drupal patch bot fail with this.

Cheers.

Aciid’s picture

Status: Closed (cannot reproduce) » Active
FileSize
494 bytes

Also opening this issue again, since I have this on our production environment and it's reproduceable. The attached patch fixes the bug. Please attach it into core.

Drupal patch bot didn't handle the file since the ticket was closed let's try again. with issue open this time

joachim’s picture

Status: Active » Postponed (maintainer needs more info)

> it's reproduceable

Could you explain how please?

AFAIK $flag->options is always set.

Aciid’s picture

Actually I got it after we cleared our server's caches with drush. But as of vinoth.3v's I'm not sure.
Mine was completely random.

But as said, the patch fixed the issue so, I think it should be included.

Thanks

joachim’s picture

Before I just commit a patch I need to understand the problem and reproduce it for myself.

> Actually I got it after we cleared our server's caches with drush.

> I am getting Fatal error: Unsupported operand types in flag/includes/flag/flag_flag.inc on line 130

When do you get the error? Is that message output by drush when clearing caches, or when you do something after? What is it you do?

Please give me precise steps to reproduce this -- that's the only way this patch can get in.

szantog’s picture

Status: Postponed (maintainer needs more info) » Active

.@joachim: I can confirm, this is a random cache related bug.

This problem comes from flag_get_link_types() We are using memcache, and sometimes $cache = cache_get('flag_link_type_info') should be something TRUE shit, against an array. Then the $options = array_merge($options, $link_type['options']); get killed in $flag_options = $flag->options(); I'm not sure, it comes from flag module, but it would be nice, some array checking in flag_get_link_types().

joachim’s picture

Might this be to do with #534092: cache_get() returns expired cache items?

I just had this filed on another project and it seems like it could be the same problem: #534092: cache_get() returns expired cache items

dixon_’s picture

Status: Active » Reviewed & tested by the community
FileSize
608 bytes

@joachim Yes, I can confirm it has to do with that. I've run into the same problem here.

So, the correct fix is to re-fetch link types if the cache is empty. The attached patch works flawlessly for me, so I'm gonna be bold and RTBC it myself since it's a trivial fix ;-)

joachim’s picture

Status: Reviewed & tested by the community » Fixed

Thanks!

Committed.

git commit -m "Issue #1925922 by dixon_: Fixed retrieval of flag_link_type_info cache producing empty flags." --author="dixon "

Status: Fixed » Closed (fixed)

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

rogerbinny’s picture

Version: 7.x-3.x-dev » 7.x-3.1
Status: Closed (fixed) » Needs review

I believe this needs to be reviewed again.

I had installed Flag 7.x-3.1 on a local installation of a Drupal 7 site. The module includes the patch in comment #15 but it is giving the same error message and bringing down the site.

joachim’s picture

Status: Needs review » Postponed (maintainer needs more info)

What's your line number?

rogerbinny’s picture

Its occurring on line #130

rogerbinny’s picture

I am also seeing a

Deprecated function: Call-time pass-by-reference has been deprecated in drupal_load() (line 1105 of public_html/includes/bootstrap.inc).

in watchdog just before the error messages. I think its failing to get the link types from the cache bin. I am not sure why.

Renee S’s picture

Confirmed, I'm seeing this too. (eta: and the patch doesn't fix it; it takes my site from a server error to a generic Drupal error.)

Renee S’s picture

Symptoms: Drupal would think for a long time, and then give me a server error (line #130 as above). Disabling Entity Cache finally brought my site back. I think something got messed up and then cached; but normal cache clears didn't fix it.

rogerbinny’s picture

I believe I found the cause and the solution to the problem.

The module file flag.flag.inc file found in the root of the file was not being called for some reason. The file contains the hook

flag_flag_link_type_info() which is being called in flag.module within the function
flag_get_link_types()

The solution would be to add the file to hook_init of the flag module like below.

function flag_init() {
  module_load_include('inc', 'flag', 'includes/flag.actions');
  module_load_include('inc', 'flag', 'flag.flag');
}
joachim’s picture

> The module file flag.flag.inc file found in the root of the file was not being called for some reason

> The solution would be to add the file to hook_init of the flag module like below.

The presence of this file is registered with hook_hook_info(). There is no need to load it directly.

Did you clear your cache?

rogerbinny’s picture

Yes, tried clearing the cache but the error was persisting.

I got the error message once again bringing down the whole site when i tried enabling devel via drush . So I went out and commented out the following lines

  //$items['flag_fetch_entity_by_user'] = $items['flag_fetch_node_by_user'];
  //$items['flag_fetch_entity_by_user']['label'] .= ' '. t('(Legacy)');

in flag.rules.inc as drush was pointing to the function flag_fetch_entity_by_user.

I guess that removed those messages permanently.

joachim’s picture

I'm afraid I'm not following the flow of code here.

How is drush calling flag_fetch_entity_by_user()? Which functions are in the flag.inc file which are needed, and by what?

If functions in the flag.inc are being called directly rather than by invoking a hook, then that's a problem.

HydroZ’s picture

I had the exact same error on line 130 in file flag_flag.in (unsupported operand types). I am using the latest devel-version which includes the above comitted patch. It did not solve the issue.
In my case it was necessary to truncate the cache tables by SQL commands (TRUNCATE table).

rogerbinny’s picture

Drush was not calling flag_fetch_entity_by_user.

I am sorry if I got you confused.

I just detected the error when using drush in the command line which prompted me to look into the code. I was not able to find the source of the error using other means. Tried the apache error log but it was coming up with the same error msg that brought me to this thread.

Drush just proved to be more descriptive with the error message in this case.

I am assuming those two lines of code are there for backward compatibility and was conflicting with the rules module.

dtabirca’s picture

We have the same project with identical database on different servers with very similar environment (all with php 5.3).
On some systems only, this error "Unsupported operand types in flag_flag.inc" appeared after installing the flag module, then all the functionality got broken. We've reinstalled everything, analized the code, tried the patches and all the other suggestions. Nothing worked for us.
After few days of investigations we finally found out that "allow_call_time_pass_reference" needs to be On in php.ini to avoid this error. I hope this will work for others as well.

yourlefthand’s picture

setting "allow_call_time_pass_reference" brought flag back for me as well. I still get

[14-Nov-2013 20:52:19 UTC] PHP Fatal error: Unsupported operand types in /var/www/sites/all/modules/flag/includes/flag/flag_flag.inc on line 130

in the logs quite a bit.

joachim’s picture

It's great that you've found something, but that's only masking the problem, not fixing it. And also, that PHP directive is going away quite soon.

Could people who have this problem please do some actual debugging to find out what the code is trying to do at that point?

    // Populate the options with the defaults.
    $options = (array) unserialize($row->options);
    $options += $flag->options();

Is $options not an array? If not, why not?
Has $flag->options() not returned an array? If not, why not?

yourlefthand’s picture

I understand that it is masking the problem and that it will not work forever, but it is a necessary workaround.

It is difficult to troubleshoot as I can't have our production site WSOD. I don't see the problem on our test site that is a clone of production. The unsupported operands error doesn't seem to happen after any particular event, but it does happen more frequently after a cache clear.

joachim’s picture

> It is difficult to troubleshoot as I can't have our production site WSOD

Interesting!

Could it be due to PHP versions?

> It is difficult to troubleshoot as I can't have our production site WSOD

Could you chuck in some code to watchdog or dump variables when that array is found to be empty?

yourlefthand’s picture

I'll see what I can do.

oscardax’s picture

I have just had this issue today after unrolling a newer version of my development site into my production one.
And the code from #2 made the fatal error disappear. Thank you vinoth.3v!!!

yourlefthand’s picture

OK, this issue white-screened my dev instance this morning so I spent a little time playing around.

I changed the flag->options() function to the following and the errors went away:

// Merge in options from the current link type.
$link_type = $this->get_link_type();
if (is_array($link_type['options'])){
$options = array_merge($options, $link_type['options']);
}

It seems that $link_type['options'] is not always an array.

yourlefthand’s picture

I am also getting the following notice:

Notice: Undefined index: normal in get_link_type() (line 1145 of /var/www/sites/all/modules/flag/includes/flag/flag_flag.inc). =>

joachim’s picture

@yourlefthand: is there any chance you could debug those variables and find out where they are actually coming from?

We need to know why the variable is not properly set. Who provided it? Which link type and/or flag is currently being looked at when it goes wrong?

yourlefthand’s picture

I'll try to look at it some more a little later - I have some other work that has to get done.

Is it typical that this code would be executing when building a page unrelated to any flag types that are configured?

joachim’s picture

> Is it typical that this code would be executing when building a page unrelated to any flag types that are configured?

If your page was doing something that cleared the cache, then the various flag caches would probably get rebuilt.

VVS’s picture

I have the same error after install module entityreference.

jnicola’s picture

Same issue!

PHP 5.4 if that matters any.

( ! ) Fatal error: Unsupported operand types in /var/sites/fsg.dev-vm/sites/all/modules/contrib/flag/includes/flag/flag_flag.inc on line 130
Call Stack
#	Time	Memory	Function	Location
1	0.0000	234136	{main}( )	../index.php:0
2	0.1187	20223080	menu_execute_active_handler( )	../index.php:21
3	0.1188	20307704	call_user_func_array ( )	../menu.inc:517
4	0.1188	20307872	user_page( )	../menu.inc:517
5	0.1188	20306584	menu_execute_active_handler( )	../user.pages.inc:550
6	0.1210	20775952	call_user_func_array ( )	../menu.inc:517
7	0.1210	20776288	user_view_page( )	../menu.inc:517
8	0.1210	20776328	user_view( )	../user.module:2549
9	0.1210	20776488	user_build_content( )	../user.module:2588
10	0.1352	21958480	module_invoke_all( )	../user.module:2641
11	0.1356	21978888	call_user_func_array ( )	../module.inc:895
12	0.1356	21979408	flag_user_view( )	../module.inc:895
13	0.1356	21979696	flag_get_flags( )	../flag.module:1177
14	0.1379	22260752	flag_flag::factory_by_row( )	../flag.module:1830
jnicola’s picture

I went to line 129, var_dumped $options, and exited. Here's what I am getting:

array (size=15)
  'flag_short' => string 'Save to my Resources' (length=20)
  'flag_long' => string '' (length=0)
  'flag_message' => string 'Saved to my Resources' (length=21)
  'unflag_short' => string 'Remove from my Resources' (length=24)
  'unflag_long' => string '' (length=0)
  'unflag_message' => string 'Removed from my Resources' (length=25)
  'unflag_denied_text' => string '' (length=0)
  'link_type' => string 'toggle' (length=6)
  'weight' => int 0
  'show_in_links' => 
    array (size=6)
      'full' => int 0
      'teaser' => int 0
      'rss' => int 0
      'search_index' => int 0
      'search_result' => int 0
      'token' => int 0
  'show_as_field' => int 1
  'show_on_form' => int 0
  'access_author' => string '' (length=0)
  'show_contextual_link' => int 0
  'i18n' => int 0
jnicola’s picture

I also dumped Flag, since line 130 does this $options += $flag->options();

object(flag_broken)[389]
  public 'fid' => string '1' (length=1)
  public 'entity_type' => string 'node' (length=4)
  public 'name' => string 'user_resource_flagging' (length=22)
  public 'title' => string 'User Resource Flagging' (length=22)
  public 'global' => string '0' (length=1)
  public 'types' => 
    array (size=0)
      empty
  public 'roles' => 
    array (size=2)
      'flag' => 
        array (size=0)
          empty
      'unflag' => 
        array (size=0)
          empty
  public 'errors' => 
    array (size=0)
      empty
jnicola’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Postponed (maintainer needs more info) » Needs review
FileSize
679 bytes

Uploading my patch for the issue at hand. Per my comments above, the object of $flag did not contain options, so it was trying to += an array with null, and PHP shit itself.

First checking if you actually were dealing with an array at all for $flag->options resolved the issue for me entirely... or at least stopped throwing errors and tags still work?

Status: Needs review » Needs work

The last submitted patch, 46: line-130-fixer-upper-rater-19252922-46.patch, failed testing.

joachim’s picture

Thanks for investigating!

At what line did you dump $flag?

    // ...but skip the following two.
    unset($flag->options, $flag->type);

    // Populate the options with the defaults.
    $options = (array) unserialize($row->options);
    $options += $flag->options();

We unset $flag->options at the beginning.

But the thing we merge in is $flag->options() -- a method call, not a property. And all the flag classes return an array for that method.

Can you add a call to $flag->options() somewhere around there and dump the return value?

jnicola’s picture

All of my dumps were performed prior to line 130, where the fatal error was occurring.

I'll try and find some time to play with this some more. I don't know if the error will persist anymore though now that things have been good for a bit?

Renee S’s picture

The solution in #46 got my site back up... Thanks much!

jnicola’s picture

FileSize
617 bytes

New patch submitted that is relative to the module itself and not relative to my site root from the site I am working on.

jnicola’s picture

Attempted to remove the code I added to get error again and output what you requested. Error is no longer persistent.

Seems just like some corrupted data causes this issue, and once you force your way past the corruption it all sorts itself out anyways, since I can no longer reproduce the issue.

jnicola’s picture

FileSize
555 bytes

Reuploading patch one final time. I know the previous version will fail. Forget to adjust both first diff line and notes on line 3 and 4.

joachim’s picture

> Seems just like some corrupted data causes this issue, and once you force your way past the corruption it all sorts itself out anyways, since I can no longer reproduce the issue.

Might this be to do with upgrading from an older version?

Or might something else have caused corruption?

jnicola’s picture

Pretty sure it's because I fed the Gremlins after midnight...

I have no idea what may have caused the issue honestly. It occurred for me while exporting the database from DEV to move to staging. Brought the DB to staging and the entire site worked great and was happy as a clam. DEV though tanked and wouldn't work. Identical databases, identical code, only difference I could spot is PHP 5.3 vs 5.4.

Renee S’s picture

This has happened to me on restore and prod-to-dev moves also. Not always, but sometimes. Same PHP (5.3) in our case, though.

rwohleb’s picture

The commenter in #24 was correct about the flag.flag.inc file not getting loaded, but not about why. If the module_implements variable in the cache_bootstrap table becomes incomplete (not sure why), you can end up with it not having the flag module listed. As a result, Drupal knows nothing of the hook_hook_info in flag.flag.inc when it tries to do a lookup in the module_implements() function.

Normally this isn't a big deal since it would be fixed with a cache flush. However, in this case you can end up with it not being able to bootstrap far enough to get to the cache flush mechanisms (including in Drush). The rules module tries to build a rules cache in its hook_init(), which eventually calls the flag module's hook_rules_event_info(), which ends up calling flag_get_flags( ).

The issue we are seeing is being caused by flag_get_link_types() returning an empty array. The flag.flag.inc file is not getting loaded so the hook with the default link types is not run. As a result, the $flag->get_link_type() method returns nothing.

When the $flag->options() method is called it does the following:

    // Merge in options from the current link type.
    $link_type = $this->get_link_type();
    $options = array_merge($options, $link_type['options']);

Array_merge() does strange things when one of the parameters is not an array. In this case it's returning NULL.

The only real solution that I see is to move the contents of flag.flag.inc into flag.module (or do an include) so that they are always available. Otherwise, the flag module's hook_rules_event_info() implementation would have to be rewritten to not rely on autoloading via hook_hook_info.

joachim’s picture

That's some pretty good detective work!

I've enabled Rules on my test site, but I can't yet reproduce this.

Do I perhaps need to have a rule that is triggered on the 'init' event?

Renee S’s picture

joachim, it's oddly intermittent. It seems to happen more on fully-loaded sites with tonnes of modules, on a first site load after a restore, or after enabling a module ...probably because there's a bunch of hooks firing all over the place from an empty cache and a router rebuild, or something like that...

si.mon’s picture

I'm having the same issue.
FYI, fixing the issue by forcing an array, then clearing cache, then rolling codebase back seems to work on my side.
However, I know this won't ultimately help finding out what's going on, but here's a patch with what was suggested by #24 and seems to be confirmed by #57. Seemed to be the less ugly solution for now.

joachim’s picture

  1. +++ b/flag.info
    @@ -6,7 +6,6 @@ configure = admin/structure/flags
    -files[] = includes/flag/flag_flag.inc
    

    That line needs to stay. It declares a file that holds classes.

  2. +++ b/flag.module
    @@ -452,6 +452,7 @@ function flag_help($path, $arg) {
    +  module_load_include('inc', 'flag', 'flag.flag');
    

    If we're doing this, then we no longer need our hook_hook_info().

    But the thing is, modules that extend Flag with custom flag or link types will continue to cause the problem, as they are expecting hook_hook_info() to do its work. So just loading the include file is not going to work.

rwohleb’s picture

@joachim, the issue isn't the Rules module or having a rule defined. The Flag module tries to interrate over all defined flags to expose them as events to Rules. When it tries to load all of the defined flags, it is expecting that it will always get at least the default link types. Since the default link types are loaded via the flag_link_type_info hook, and then modified by the flag_link_type_info alter hook, the Flag module could get any combination of results due to other modules (or incomplete cache in this case, possible due to server load during cache rebuild). Either the default link types are included statically every single time, or it needs to be able to handle the case where none are defined. Even if the default link types are defined statically, if they end up being run through the flag_link_type_info alter hook, you could still get other modules removing them.

If you want to duplicate this, you should be able to do so be removing the flag module entry in the module_implements array stored in the cache_bootstrap table.

joachim’s picture

> removing the flag module entry in the module_implements array stored in the cache_bootstrap table.

I'm really confused as to how that could happen. If the module_implements cache doesn't have an entry for a hook, then it consults hook_hook_info() and updates itself: https://api.drupal.org/api/drupal/includes!module.inc/function/module_im...

Icicle2dx’s picture

Quick Temporary fix that worked for me.

Comment out Line 130.

//$options += $flag->options();

run Drush CC all

Uncomment Line 130.

This occured for me after I enabled a custom module which had an error in the block_view hook (I referenced the incorrect function) Then everything borked.

Hope that helps!

si.mon’s picture

On my side it appears that flushing cache a couple of times does the job. Flushing just once doesn't work. Really weird !

Renee S’s picture

This is the oddest error, and I wonder if it's something combined with Entity Cache as well, given its transience?

P2790’s picture

Thanks Icicle2dx,

That worked for me, I came across this problem when patching the fape module with https://drupal.org/comment/8370033#comment-8370033

I rolled back the patch and then did https://drupal.org/comment/8311685#comment-8311685

Shabbir’s picture

hey guys,

comment #2 (or the patch at #8 by Aciid) worked for me, I wrapped the if condition around the statement " $options += $flag->options(); " (on line 130 in flag_flag.inc) and it worked for me, Thanks alot guys. I would be obliged if any of you can give an explanation on why that if condition was required and why all of a sudden the issue came. Still learning drupal :)

I had a drupal 6 site which I migrated to drupal 7 (version is 7.24) with flags 7.x-3.2

Regards,
Shabbir

joachim’s picture

> I would be obliged if any of you can give an explanation on why that if condition was required and why all of a sudden the issue came.

We need people who actually have this problem to figure out why it's happening on their systems!

rwohleb’s picture

@joachim, the issue wasn't that the hook had no entry in the module_implements array, it was that the flag module's entry was missing. This issue appears under certain load conditions that can cause corrupted cache data. As i said before, even when all the hooks fire correctly, there is still the possibility of another module doing an alter to remove the default link types. The code is just assuming the defaults will always be available and not doing a sanity check.

joachim’s picture

Title: Unsupported operand types in flag_flag.inc » Unsupported operand types in factory_by_row() in flag_flag.inc

> there is still the possibility of another module doing an alter to remove the default link types. The code is just assuming the defaults will always be available and not doing a sanity check.

Modules shouldn't do something like that! Or at least, if you code a module that does that, you'd better not be using the link type you remove!

Also, hook_flag_link_type_info() documents that it requires the 'options' property to be set, and to be an array. Any module implementing hook_flag_link_type_info() or hook_flag_link_type_info_alter() and NOT providing an array there is misbehaving.

jnicola’s picture

Status: Needs work » Needs review

Why is this marked needs work?

We've got a handful of people with the issue, 4 patches that all fix the solution... just review the solution and implement it! It's a no brainer fix that prevents a PHP fatal error!

The last submitted patch, 60: flag-module_load_include-1925922-60.patch, failed testing.

joachim’s picture

Status: Needs review » Needs work

The maintainer cannot reproduce the problem.

> All of Flag's flag classes return an array for options(), so I don't see how this warning could come up.

joachim’s picture

To expand on that:

- Step 1: We need clear steps on how to reproduce the problem. We don't have those yet.
- Step 2: Figure out where the problem is originating from. That property should never not be an array, so if it sometimes is, it's because something somewhere else is going wrong -- either in Flag or in another module.
- Step 3: Fix that, rather than hack around it.

antondavidsen’s picture

joachim’s picture

Does anyone even read my comments????

bluewallmedia’s picture

@joachim,

I was having very similar issues as those described in this thread. I had to go back and read through all of the comments here after your last request to read them. I totally understand the the issues I was experiencing might not be coming from the module itself. But here is what I can tell you.

I kept getting this error and WSOD killing the site.

Unsupported operand types in /sites/all/modules/flag/includes/flag/flag_flag.inc on line 130

I was experiencing this issue on a Ubuntu server running PHP 5.3.2-1ubuntu4.23 with the following directive 'allow_call_time_pass_reference' set to OFF. I did not need to enable it to fix my problem on my server.

I had moved exported and imported my database around on the same server running through the normal dev, staging, etc. I had thought that my problem was related to running this module in dev and then needing to upgrade it. It was upgrade where I started having issues. So I know. a) I had old related modules which had been installed in database, were not un-installed and where waiting to be reinstalled in admin/modules.

I can confirm that the these two patches worked for me to get out of WSOD land and at least back to where I can see the site and work on the issue.

#51 https://drupal.org/comment/8220341#comment-8220341

and

#76 https://drupal.org/comment/8597029#comment-8597029

Hope this info helps. My intuition tells me I created this problem during a long upgrade with bad data and uninstalled modules. Though reinstalling modules which had been turned off during upgrade did not fix my issue but the patches did bring the site back to life and make me a happier camper.

Thanks to everyone for the help. Thanks to Drupal for brining us all together.

~ peter

jnicola’s picture

Do you still have the database dump that caused the WSOD?

If so, would you be willing to share it? I'd like to compare the flag tables of a DB that causes issues alongside a healthy DB. I bet the issue lies somewhere in there!

hugues.esslinger’s picture

Hi,

Excuse me for my English, I'm French.
I had the same problem...

First For me :
#51 https://drupal.org/comment/8220341#comment-8220341
and
#76 https://drupal.org/comment/8597029#comment-8597029

Create the illusion that Flag module works right But it make more other conflict... Because the real reason of this bug aren't fix...

#30 "allow_call_time_pass_reference" Fix the PB for me but I think is not a good deal...

So for fix it I used devel module.

I use 3 modules :

Flag,
Webform,
and my module custom.

And finaly the probleme was in my custom module... In fact i make bulshit... In my custom module I work in a hook_form_alter. And I created a personal function like this :

function my_module_form_alter(&$form, $form_state, $form_id) {
if($form_id=='[myFormId]'){
/** 
Here is my big ERROR !!! &$form is bulshit ! when I call my function... I must be tired...
I past $form in reference and the result is in flag.module and function flag_fetch_definition :
$cache->data is empty and make flag_broken...
When I fix it as _my_module_hidden_field($form, $form_state); Flag module run normaly...
**/
_my_module_hidden_field(&$form, $form_state); // Here my ERROR...
}
}

function _my_module_hidden_field(&$form, $form_state){
// personal code...
}

Hoping I could help you in fixing this bug.

Marc Angles’s picture

I had this issue tonight. When enabling a custom module that implements hook_block_info and hook_block_view.

I don't know if this is related to these functions... but

As soon as I enable the module (I tried a second time with the example_block.module and had the same bug), I have the WSOD

Unsupported operand types in /sites/all/modules/flag/includes/flag/flag_flag.inc on line 130

I'm running flag-7.x-3.4

To have my site back I discovered that I could run

drush rr && drush dis my_module -y

Then the site is back.

Just wanted to share for those who can use drush

joachim’s picture

I just tried enabling example_block.module on my test site and it works fine...

rwohleb’s picture

@joachim, it's not that your code is inherently faulty, it's that it assumes everything will always be perfect. It assumes that it will always get a valid input. That is where the problem originates. In comments #57, #62, and #70 I detailed the input conditions that create the WSOD. There are any number of conditions, caused by hardware, core, and contrib, that would give your code faulty input. All we are asking for is that you add a couple of sanity checks to the code so that it no longer explicitly trusts the input. This doesn't change how the module functions at all, but makes it more flexible in a complex environment where many other contrib modules (with their own bugs) exist. Hopefully this is a better explanation of our goal.

I've only been able to reproduce this in an environment with real customer data and a large number of modules enabled. As such, I can't provide a DB dump for you to analyze. Wish I could.

joachim’s picture

Can you reproduce it reliably? Can you find the source of the problem?

This thread has seen so many theories that I've totally lost track -- there's been things about the cache not providing data, or pass-by-reference, but I don't see how those would be causing this.

    // Merge in options from the current link type.
    $link_type = $this->get_link_type();
    $options = array_merge($options, $link_type['options']);

If you get this far and $link_type['options'] is not set **then your flag is broken** and crashing is the correct course of action.

kevinquillen’s picture

Version: 7.x-3.1 » 7.x-3.3

crashing is the correct course of action.

Straight up crashing is never the correct course of action if it can be avoided.

I just handled a call for a non-responsive site that was WSOD and led me to this very page. Patching line 130 to not assume that $flag->options(); always returns an array would be correct - as you say other contrib modules can hook in with alter hooks and potentially muck up the options. Fatal errors are not preferred, at least insulate against them.

The only course I could take was logging in with a SQL client and truncating all the cache tables - the site came up then. So this problem pertains to both that line, and something that gets stored in cache (possibly broken cache_bootstrap table as was mentioned). Without a SQL client, it totally locks someone out of the site. After patching line 130, that part did not come back. Then, this happened on drush cc all:

ReflectionException: Function flag_fetch_entity_by_user() does not exist in ReflectionFunction->__construct()  [error]
(line 1691 of
/docroot/profiles/basic/modules/contrib/rules/includes/rules.core.inc).
Drush command terminated abnormally due to an unrecoverable error.                                                     [error]
ReflectionException: Function flag_fetch_entity_by_user() does not exist in ReflectionFunction->__construct() (line 1691 of /docroot/profiles/basic/modules/contrib/rules/includes/rules.core.inc).

Can this be patched to prevent both the form and the cache errors so it at least handles a 'broken' flag gracefully instead of WSOD an entire site? It should report broken flags to you and not crash out your site.

Additional errors when doing drush cc all:

marketing35@srv-2811:/vol/ebs1/gfs/home/marketing35$ drush @marketing35.test dis rules_ui
array_merge(): Argument #2 is not an array flag_flag.inc:199                                                                                                                                     [warning]
Invalid argument supplied for foreach() flag_flag.inc:218                                                                                                                                        [warning]
array_merge(): Argument #2 is not an array flag_flag.inc:199                                                                                                                                     [warning]
Drush command terminated abnormally due to an unrecoverable error. 
joachim’s picture

If someone can show me how in the code flow we're failing to get arrays at the point of the crash, I'll commit the patch, or better yet, I'll fix the cause of the lack of an array. Without that, this is just babysitting someone else's broken code.

> Can this be patched to prevent both the form and the cache errors so it at least handles a 'broken' flag gracefully instead of WSOD an entire site?

There is a broken flag class. Is this getting used here?

jnicola’s picture

Status: Needs work » Needs review

I extremely dislike that you're holding the issue queue hostage despite plenty of completely viable solutions with a demonstrable history of success with them... but I'll play your game...

Obviously no error class is being leveraged as it's bringing a ton of people here with a WSOD issue, so whatever error handling conceptualized prior to this is not working.

Would you accept a patch that instead of just bypassing the check that incorrectly assumes something will always be correct, avoids a WSOD and errored out in an acceptable fashion either via drupal standards or leverage your broken flags class?

joachim’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Do feel free to go take a look at how many issues I've committed patches for and closed in the last few weeks, and then tell me again how I'm holding the issue queue hostage.

    $options = (array) unserialize($row->options);
    $flag_options = $flag->options();
    if (!empty($flag_options)) {
      $options += $flag_options; // THIS LINE, RIGHT?
    }

All we seem to know is that the marked line produces a crash. Has anyone even determined which of the two variables is causing the problem? Has anyone tried to track it back further, with debug statements in the code that is setting that variable earlier on? This is how we fix bugs, not by just coding around them with hackjob sticky plaster fixes.

jnicola’s picture

I have gone through a stack trace trying to figure out what happened. It yielded no results. I've tried going into the cache tables and putting in completely erroneous data... and it still proceeds fine.

The problem with reproduction is that as soon as you cure the problem (a larger priority since this is occurring for all of us on production sites) the issue cannot be reproduced.

So, since we can't proceed with "how we fix bugs", would you proceed with a "how we avoid preventable fatal errors" approach, that gracefully handles an error at this step, instead of "coding around them with hackjob sticky plaster fixes."

kevinquillen’s picture

For #86 - I do not know what a broken flag class is, from what I can tell its a basic flag on a node type, connected to a product in Commerce (so its a product display).

joachim’s picture

> The problem with reproduction is that as soon as you cure the problem (a larger priority since this is occurring for all of us on production sites) the issue cannot be reproduced.

If you cure the problem with the sticky plaster fix, then the problem has not been cured at all. You're just masked it at the point where it causes a crash.

So you could still chuck a load of watchdog() statements to track those variables.

jnicola’s picture

Okay, I am no longer discussing the "sticky plaster fix". I am acting under your statement that it is unacceptable. Please, continue past that.

With an established problem, and now no acceptable solutions, I am attempting to discuss alternative solutions that you might find acceptable.

Would you accept a solution, that gracefully handled the error (Not a WSOD), and captured some data in the system log? Then whomever comes here can use that patch, get past the situation, get us some data... and possibly get what you're ideally looking for?

I think it will be key to allow the user to get past the issue (this is occurring on production sites after all and people have information to get out, widgets must sell!) and would enable the capture of useful practical data for you to review the initial issue.

Sidenote: The "sticky plaster fix" only has to exist for one run of the code. I switched the code back afterwards and everything proceeded as expected. This appears to pertain to data corruption, and once it gets past that point it must correct the corrupted data.

joachim’s picture

> Would you accept a solution, that gracefully handled the error (Not a WSOD), and captured some data in the system log?

No. That's what somebody with this problem should be doing as part of fixing this bug, so they can report back on the source of the problem.

> This appears to pertain to data corruption, and once it gets past that point it must correct the corrupted data.

I'm not sure what can cause that kind of data corruption. My only hunch is that it could be due to upgrading a site from older version of Drupal core and Flag. In which case the correct fix would be a hook_update_N() to correct the data.

kevinquillen’s picture

What data are we looking for, though? From what I can tell, this site had Flag 3.3 installed, and one flag created ("Add to List") - essentially tracks favorites. The site isn't live yet, still in testing by just a couple people. What could trigger this? Is Features exporting it incorrectly?

The site works fine up until you clear the cache (or turn on/off a module, run cron, anything that triggers a cache clear), and then you get the unsupported operand error on line 130.

joachim’s picture

Start with the lines I pasted in https://drupal.org/node/1925922#comment-8757907.

Which side of the assignment is failing to be an array?
Once you have that, track it back. Where was it set? Why was it not an array at that point?

jnicola’s picture

I would strongly argue that hook_update_n is not the correct solution. This isn't a difference between versions or anything causing the issue, it entirely relates to sporadic and largely rare data corruption.

So you're completely opposed to building in error handling (instead of a slide by fix) at this step of the code?

joachim’s picture

> it entirely relates to sporadic and largely rare data corruption.

What kind of problem would cause this though?

rwohleb’s picture

@joachim: All you have to do is go back and review my comments (#57, #62, and #70). I posted a detailed explanation of what the issue is and how to fix it. As for what causes the data corruption in the first place? As I've already said, it can be any number of things, including hardware issues causing faulty DB inserts. The current code does NOT fail gracefully, so we end up with this huge list of comments trying to convince you to add a simple sanity check in the code. Having some simple sanity checks on input is just good coding practice in Drupal contrib modules.

joachim’s picture

Could you update the issue summary with the details from those? It's now really hard to keep track of what's actually definitely been figured out about this problem.

> including hardware issues causing faulty DB inserts

We'd have to wrap everything up in sanity checks to account for hardware failures!

Renee S’s picture

This issue has 34 followers, so it's obviously something more common than solar flares...

joachim’s picture

It would be great if some of these 34 people could do some debugging, get some more details on the cause of the problem, and thus enable us to fix it cleanly.

jnicola’s picture

How's about this snippet from developers doing far better things than janky ass flags:

"Well-written applications include error-handling code that allows them to recover gracefully from unexpected errors. " Microsoft

Go google it for a bit. Everyone with 50 bajillion times more credibility than thou (or me) is all about graceful error handling. You can't predict every error, but when an error slaps you in the face 34 times... it's become tangible to provide graceful degredation...

Again: Would you accept a graceful form of erroring out at this point, or are you going to stand your ground like a six year old child some more?

joachim’s picture

Ok I am now unfollowing this issue. If anyone ever gets round to adding something constructive and useful, please email me.

kevinquillen’s picture

The behavior of array_merge() was modified in PHP 5. Unlike PHP 4, array_merge() now only accepts parameters of type array. However, you can use typecasting to merge other types.

One of the inherent problems is simply assuming that the values being assigned both exist and is an array, which, while it's supposed to exist and supposed to be an array, is simply not reliable to expect 100% of the time in a contributed, alterable ecosystem. In fact, you're already typecasting the $options variable, why not the same for the flag options?

$flag_options = (array) $flag->options();

That at least guarantees that when you do a += or array_merge, the values are empty arrays. The argument really isn't that they should never be empty, the argument is handling it better than crashing an entire website if they are. In certain edge cases, the options are NULL or empty, which does not play well with += assignment of another type. Typecasting is not a hacky fix, it ensures your data type is what you expect.

What the others are advocating for is handling this event more gracefully so it doesn't kill the world with a cryptic error. The most common thread is that clearing site cache triggers this issue, and they want room to track it further back without sitting there at a dead site for now.

jnicola’s picture

Very well put, and I like it :)

Renee S’s picture

jnicola: I think it would be productive to remove the personal attacks from your comment; I understand that you're frustrated, but it can only hurt the case we're trying to make, and this is a good, useful module that we all benefit from.

kevinquillen, you make a good reasoned argument; could you email it to joachim? I think it is useful to the issue, and the end result we'd all like is for error-handling to get incorporated here.

rwohleb’s picture

The sad part is that this issue has been explained MULTIPLE times already. The module maintainer doesn't want to commit a fix without a reproducible test case, though that's not always feasible in complex systems. Even though the request is just for a quick sanity check to be added to the code, the module maintainer has now unsubscribed from this thread. Very sad state of affairs.

rwohleb’s picture

Issue summary: View changes
rwohleb’s picture

I updated the initial issue text with a summary of the bug, a link to specific comments in this stream, and an explanation of why this remains unresolved. Maybe someone will read it and come up with a repeatable test case that @joachim will actually accept.

This is very frustrating, and I'm really having to bite my tongue about all of this :(

joachim’s picture

Status: Needs work » Postponed (maintainer needs more info)
FileSize
1.8 KB

Thanks for the first pass at the summary.

More detail is needed though, and in the body of the summary itself rather than as links to comments. (See the issue summary docs page for a template for writing a summary.)

Regarding the theories linked in the summary:

> However, in this case you can end up with it not being able to bootstrap far enough to get to the cache flush mechanisms (including in Drush). The rules module tries to build a rules cache in its hook_init(), which eventually calls the flag module's hook_rules_event_info(), which ends up calling flag_get_flags( ).

That's plausible, but we'd need to have the actual rules involved. With Rules OOTB, this all works fine, so presumably it's a particular rule's combination of elements that causes this.

> Since the default link types are loaded via the flag_link_type_info hook, and then modified by the flag_link_type_info alter hook, the Flag module could get any combination of results due to other modules (or incomplete cache in this case, possible due to server load during cache rebuild).

If Drupal is not properly aware of which modules implement which hooks during a cache clear, then your whole site is broken. Flag module is the least of your worries.

> That at least guarantees that when you do a += or array_merge, the values are empty arrays

The code guarantees that they are at least empty arrays. If someone can read the code and explain why that's not the case, then we can fix that.

Now, it's been some weeks since I asked for people experiencing this bug to do some debugging, and nobody seems to have taken then time to actually do that. So here is a patch that does that. It's taken me about 5 minutes to write it, and I'm thoroughly disappointed nobody has beaten me to it -- even though plenty of you have time to post comments.

We need people experiencing this bug to apply the patch, and try to reproduce the problem with it. You still have the problem, even if you've applied one of the earlier patches, because all they do is mask it. So your sites are continually hitting this problem, and creating $flag objects that do not have their options properly set up. With this patch, maybe we can actually get to the bottom of this and fix it properly.

I'm setting this issue to 'needs info'. Please do not post any further comments unless they provide the actual concrete information that's needed.

antiorario’s picture

FileSize
1.93 KB

This is an updated version of joachim's debug patch, which is missing an argument in watchdog(), and creates a fatal error when using database logging.

antiorario’s picture

FileSize
1.94 KB

Rerunning my own patch, because I missed a bit.

joachim’s picture

Oops!! Thank you! :)

antiorario’s picture

I have good news and bad news. The good news is that I was able to fix the bug on the site that was affected, and also that Flag wasn’t directly responsible for it. The bad news is that the responsible module is one I custom-coded for this particular site.

The cause of it all was that in this custom module I’d decided to define a constant as the return value of a function (I never thought it was a good idea, and now I know it was just dumb). To do so, the module called module_load_include() on one of its dependencies (also a custom module), so that the required function would be available to define(). In turn, this included file defines and calls a few hooks, which I guess is what was getting in the way of the normal hook-caching process during bootstrap.

I got rid of that constant (and of module_load_include()), and the problem has gone away.

I know this won’t help too much because it doesn’t involve modules that anyone else is using. However, I hope it can shed some light onto the kind of stuff that might interrupt a normal bootstrap. Apparently Flag had little to do with it—it just got caught in the crossfire.

joachim’s picture

Thanks for debugging and reporting back!

rwohleb’s picture

Issue summary: View changes

@joachim: I did not create this issue, so I am hesitant to go rewriting it.

rwohleb’s picture

@joachim: You've got your rewritten summary. It's all of the information that was already available in the comments, but can now be found by someone who doesn't want to read the long comment thread. Hopefully this new format is one that will let you grok what we've been trying to say the entire time. The problem is not complex and the solution is insanely simple.

Ignoring the corrupted cache situation, let's just look at it from the alter hook angle. You provide an alter hook that any developer can use, yet you assume that you will always get good data from it. Just like you shouldn't ever trust user input (SQL injection anyone?), you shouldn't explicitly trust other module developers to never pass you data that leads to a fatal error. No one expects the Flag module to somehow magically make sense of bad input, we just don't expect it to completely crash a website to the point that even a cache flush no longer works.

I'm sorry for my frustration, but it has felt like you've just been asking us to jump through hoops the entire time instead of trying to work with us. I've been in this community a long time, and I honestly can't understand the resistance we've been experiencing.

joachim’s picture

Thanks for taking care of the summary. It's fine that you didn't report it originally; that's how we're now using the first post of an issue on d.org: anyone can edit it and should improve and update it as necessary.

> You provide an alter hook that any developer can use, yet you assume that you will always get good data from it. Just like you shouldn't ever trust user input (SQL injection anyone?), you shouldn't explicitly trust other module developers to never pass you data that leads to a fatal error.

Wrong. An API is a contract. If module A exposes an API, and module B makes use of it, then the onus is on the developer of module B to correctly understand the contract, and correctly call or make use of the API. This is a principle you'll find throughout Drupal core.

If we had to continually had to check for what hooks and API calls returned, our code would be littered with is_empty() and isset() and would be very much harder to read.

(There is also an obligation on the developer of module A to clearly and correctly document the API so that it can be understood. I am fairly confident in the documentation for Flag's hooks.)

> but it has felt like you've just been asking us to jump through hoops the entire time instead of trying to work with us

Hardly! I've even provided a patch for you to use for getting to the bottom of this, because apparently nobody with this problem can be bothered or is capable of doing some basic debugging! Find the source of the error, and report. That's all I have to say.

JulienF’s picture

Version: 7.x-3.3 » 7.x-3.5

Hello There,

I'm having the same issue with one of my sites while switching from a local environment to a test environment.

Fatal error: Unsupported operand types in /modules/flag/includes/flag/flag_flag.inc on line 132

var_dump($options); =>
array(22) { ["flag_short"]=> string(12) "Report claim" ["flag_long"]=> string(0) "" ["flag_message"]=> string(0) "" ["unflag_short"]=> string(13) "remove report" ["unflag_long"]=> string(0) "" ["unflag_message"]=> string(0) "" ["unflag_denied_text"]=> string(8) "Reported" ["link_type"]=> string(4) "form" ["weight"]=> int(0) ["form_flagging_help"]=> string(23) "Why are you reporting ?" ["form_flagging_button"]=> string(6) "Report" ["form_flagging_delete_button"]=> string(0) "" ["form_unflagging_help"]=> string(0) "" ["form_unflagging_button"]=> string(0) "" ["form_unflag_link_leads_to_edit"]=> int(0) ["form_interaction"]=> string(15) "flagging_dialog" ["show_in_links"]=> array(8) { ["full"]=> string(4) "full" ["teaser"]=> string(6) "teaser" ["rss"]=> string(3) "rss" ["search_index"]=> string(12) "search_index" ["search_result"]=> string(13) "search_result" ["charity_giveaway_fund"]=> string(21) "charity_giveaway_fund" ["gallery"]=> string(7) "gallery" ["token"]=> string(5) "token" } ["show_as_field"]=> int(0) ["show_on_form"]=> int(0) ["access_author"]=> string(0) "" ["show_contextual_link"]=> int(0) ["i18n"]=> int(0) }

$flag_options = $flag->options();
var_dump($flag_options); =>
NULL

The site takes ages to load any page (because I need to disable the nodejs integration modules since I don't have a nodejs server working in the test env.) so if you need more information to debug properly try to ask them in a grouped way so I can get you all the info with one single call.

Hope this helps and ready to dig this more (but after I get some sleep for now as it's a bit late by my side of the world ;-),

joachim’s picture

Excellent, some debugging! Thank you :)

The relevant code is:

    // Populate the options with the defaults.
    $options = (array) unserialize($row->options);
    $options += $flag->options();

So the debugging tells us:

var_dump($options); =>

The $options variable holds a properly-formed array. So the $row loaded correctly from the database. So it's NOT database gremlins / solar flares / data corruption.

 $flag_options = $flag->options();
var_dump($flag_options); =>
NULL

We're one step closer to the problem! That's great!

options() has returned NULL rather than an array. Why?

I can think of two possibilities:

* your $flag object is of a subclass that is overriding options() and doing something wrong. If you could var_dump() $flag and find out what class it is, we could potentially eliminate this possibility.
* something is going wrong in flag_flag::options(). And options() begins by defining the variable it will return as an array. If the problem is here, then something happens to it further on. We perform an array_merge() on it, but array_merge() always returns an array. The other thing we do to it is pass it off to drupal_alter(). Here, any module implementing the hook could completely change $options. So that's another possibility.

If you're able to do some more digging, which would be great, here are the two avenues to explore:

* var_dump($flag) around line 132, and report back which class it is.
* do some var_dump()ing in options(), before and after the call to drupal_alter()

JulienF’s picture

Hello Joachim,

Glad this is helpful. Here is what I did:

Around line 130 (function factory_by_row)

    // Populate the options with the defaults.
    $options = (array) unserialize($row->options);
    var_dump(array("flag object" => $flag));
    $options += $flag->options();

and around line 202 (function options)

// Merge in options from the current link type.
    $link_type = $this->get_link_type();
var_dump(array("Options before merge:" =>$options, "Link type[options]"=> $link_type['options']));
    $options = array_merge($options, $link_type['options']);
var_dump(array("Options before alter:" =>$options));
    // Allow other modules to change the flag options.
    drupal_alter('flag_options', $options, $this);
var_dump(array("Options After alter:" =>$options));
    return $options;

and here is the output I get (I'm putting variables in array with a string key to know what is displaying):

array(2) { ["Options before merge:"]=> array(9) { ["flag_short"]=> string(0) "" ["flag_long"]=> string(0) "" ["flag_message"]=> string(0) "" ["unflag_short"]=> string(0) "" ["unflag_long"]=> string(0) "" ["unflag_message"]=> string(0) "" ["unflag_denied_text"]=> string(0) "" ["link_type"]=> string(6) "toggle" ["weight"]=> int(0) } 
["Link type[options]"]=> NULL }

array(1) { ["Options before alter:"]=> NULL } 
array(1) { ["Options After alter:"]=> NULL } 

array(1) { ["flag object"]=> object(flag_broken)#1090 (8) { ["fid"]=> string(1) "9" ["entity_type"]=> string(4) "node" ["name"]=> string(5) "claim" ["title"]=> string(5) "claim" ["global"]=> string(1) "0" ["types"]=> array(0) { } ["roles"]=> array(2) { ["flag"]=> array(0) { } ["unflag"]=> array(0) { } } ["errors"]=> array(0) { } } } 
array(2) { ["Options before merge:"]=> array(9) { ["flag_short"]=> string(0) "" ["flag_long"]=> string(0) "" ["flag_message"]=> string(0) "" ["unflag_short"]=> string(0) "" ["unflag_long"]=> string(0) "" ["unflag_message"]=> string(0) "" ["unflag_denied_text"]=> string(0) "" ["link_type"]=> string(6) "toggle" ["weight"]=> int(0) } 
["Link type[options]"]=> NULL }

array(1) { ["Options before alter:"]=> NULL } 
array(1) { ["Options After alter:"]=> NULL } 

As you can see, it seems the options() function is called more than once and the array_merge inside it is wiping the array away. I deliberately added the $link_type['options'] variable to the var_dump showing array(2) as I guess it probably helps debugging.

I repeatedly get this error message every time I try to flush the cache using the admin menu.

If I put a casting to an array on line 132 the site opens but If I then try to flush some cache I get this error:
Fatal error: Call to undefined function addressfield_token_info_alter() in /data/disk/o5013607723/static/drupal-7.24.1-prod/includes/module.inc on line 1101
or other errors, the only way I could make it work was to cast the $link_type['options'] on line 203 to stop having both Fatal errors and warnings on the page...

Keep us posted and let me know if you need anything else.

Best,

joachim’s picture

> array(1) { ["flag object"]=> object(flag_broken)

Well that's interesting. Your flag is broken! That's something that's going wrong way before the $options merging problem.

But let's carry on with the empty $options. We have:

> ["Link type[options]"]=> NULL }

just before it goes into:

    $options = array_merge($options, $link_type['options']);

Turns out I was wrong when I said further up that array_merge() always returns an array. This test code produces a warning, and the result is NULL:

  $array_1 = array('foo');
  $array_2 = NULL;
  $merge = array_merge($array_1, $array_2);
  debug($merge);

So what we now know is that this line turns $options into NULL: $link_type['options'] is NULL. But why is that?

Your flag's link_type is 'toggle' which should be fine.

So the next steps are on two fronts:

* debug in flag_create_handler() to see why your flag is getting created as a flag_broken:
** what $entity_type is your flag for?
** what $definition is that code getting for your flag's entity type?
** why doesn't the class exist?
* debug in flag_flag::get_link_type() to see why the link type options are NULL:
** what is $link_types?

Good luck!

kevinquillen’s picture

So what we now know is that this line turns $options into NULL: $link_type['options'] is NULL. But why is that?

That's what was reported in the whole thread by just about everyone.

Turns out I was wrong when I said further up that array_merge() always returns an array. This test code produces a warning, and the result is NULL

Yep. This is near impossible for me to reproduce. It happens 1/50 times on a cache clear - but when it does, WSODs an entire site.

JulienF, could you create a public GH repo with your database so others can try to debug on the same page you are on? You're able to trigger it every time, I am not.

JulienF’s picture

I'll perform some more tests by next week as I can't now and I kept the array casting in place so the website runs for now.

I unfortunately can't put the DB on a public repo as the website I'm talking about is a work in progress for a customer with which I signed an NDA, I'll see if I can reproduce that on a different site though so I can hand you the steps to do to reproduce that, one hint I have for now is to install the emogrifier module without putting its library in the right folder (as this caused a huge mess on my site).

Best,

vinoth.3v’s picture

Version: 7.x-3.5 » 7.x-3.x-dev
Priority: Major » Critical
Status: Postponed (maintainer needs more info) » Active

Again, when updating to flags 7-3.5 I got this Fatal error. It could not update.
same error on 7-3.x-dev too. :(

I was just deleting one flag from admin page, and started getting errors, after upgrading the module, I am getting fatal error WSOD.

changing this to critical. :(

Edit: an additional errors,
Notice: Undefined index: normal in flag_flag->get_link_type() (line 1204 of includes/flag/flag_flag.inc).
Warning: array_merge(): Argument #2 is not an array in flag_flag->options() (line 205 of flag/includes/flag/flag_flag.inc).
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 'AS FROM o WHERE (o. IN ('XXXX'))' at line 1: SELECT o. AS FROM {} o WHERE (o. IN (:db_condition_placeholder_0)) ; Array ( [:db_condition_placeholder_0] => XXXX ) in flag_handler_argument_entity_id->title_query() (line 41 of flag/includes/views/flag_handler_argument_entity_id.inc).

joachim’s picture

Priority: Critical » Major
Status: Active » Postponed (maintainer needs more info)
Renee S’s picture

vinoth.3v, can you add the patch that joachim posted above (https://www.drupal.org/node/1925922#comment-8806105) and report what it says?

vinoth.3v’s picture

that Debug patch?

flag-issue-1925922 $options in options() at line 206 is not an array.
flag-issue-1925922 $options in options() at line 212 is not an array.
flag-issue-1925922 $options in options() at line 206 is not an array.
flag-issue-1925922 $options in options() at line 212 is not an array.

It is already known. options is null, It is reported when Issue was first created.
$flag object is broken object.

Renee S’s picture

Yup, it's known, but now we have the output that joachim is looking for! Which hopefully will help move this issue ahead. Thanks!

joachim’s picture

We need to go further back than that now -- see the comments following that patch.

vinoth.3v’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
590 bytes

OK, finally (I think so), I got it.

when debugging flag_create_handler, flag definitions was empty, but, it was strange that to see flag_fetch_definition also giving empty array after direct module_invoke_all('flag_type_info').
So with further reference of hook_hook_info API, I got a interesting very old issue #977052: Implement hook_hook_info() for Field API hooks.

flag.flag.inc file was not included automatically by core in very rare cases which results hook flag_flag_type_info is not called.

the patch, simply include that flag.flag.inc in flag_init.
I am not sure this is the right way.

After clearing the cache by drush, site back in action.

Edit: Still I am getting flag_broken object, ' Undefined property: flag_broken::$show_as_field in flag_entity_view() (line 960 of flag/flag.module', but, not fatal error this time.

vinoth.3v’s picture

Adding another patch for ' Undefined property: flag_broken::$show_as_field'

EDIT: Please ignore this patch. it is irrelevent to this issue.

Status: Needs review » Needs work

The last submitted patch, 132: flag-broken-1925922-131-1.patch, failed testing.

joachim’s picture

> Edit: Still I am getting flag_broken object, ' Undefined property: flag_broken::$show_as_field in flag_entity_view() (line 960 of flag/flag.module', but, not fatal error this time.

That's a separate problem, so needs a new issue. I'm surprised that a broken flag gets that far -- it should surely have removed itself from proceedings by this point.

Regarding #977052: Implement hook_hook_info() for Field API hooks, it is indeed a very old issue. module_invoke() has since been fixed.

> flag.flag.inc file was not included automatically by core in very rare cases which results hook flag_flag_type_info is not called.

If that's what's happening, then one of the following is true:

- there's a bug in core
- someone is invoking the hook at a point that's too early in the cache rebuild for core to correctly handle it

rwohleb’s picture

@joachim This is not the only issue that has seen cache poisoning. It appears the module implements cache can be corrupted if run after a cache flush and at the wrong level of bootstrap (ie. not all modules have been loaded). Looks like this issue has been fixed in D8 but remains in D7. https://www.drupal.org/node/1934192#comment-7363318

Now that you have another thread that details the conditions under which the module implements cache can be poisoned, can you agree that it actually happens? All we are asking for is that your module fails cleanly under bad bootstrap conditions so that users are able to flush the cache and brings thing back to life. If this module continues to do a hard fail you make it very difficult for a user (not always a developer) to correct the situation.

rwohleb’s picture

rwohleb’s picture

Issue summary: View changes
sgabe’s picture

I just ran into this issue... The patch in #131 worked for me.

joachim’s picture

Status: Needs work » Closed (works as designed)

Thanks @rwohleb for finding that core issue.

Clearly, efforts to fix this should be concentrated there.

rwohleb’s picture

Status: Closed (works as designed) » Reviewed & tested by the community

@joachim No, the blame for this should not be thrown on the core team. It's an unfortunate issue, but not necessarily a core bug. There is no guarantee when or if it will be fixed in D7. It's quite the norm for modules to code around issues or limitations in the core and contrib APIs.

Most of us have been trying to be extremely patient about this. As of now, most of us see this as you denying the patch out of spite. Please tell us that's not true. Once again, I'll iterate that all we want is this module to play nicely when other people's code might not be behaving very well. Right now the code in this module turns a soft failure into a hard failure due to a lack of sanity checking.

The last submitted patch, 112: 1925922.flag_.array-merge-debug-correct.patch, failed testing.

kevinquillen’s picture

Until the core is fixed, if it is fixed (with focus on D8 right now), which could take a very long time, I feel rwohleb is correct. Flag should (despite our reluctance to monkey patch) avert this issue and alleviate this notoriously difficult to identify bug until the core addresses it. Until then, there could be countless others out there enduring this and have no idea that it may be stemming from Flag.

Hell, there are even spots in core that link to issues in core that are a 'workaround until' scenario.

joachim’s picture

Status: Reviewed & tested by the community » Closed (works as designed)

There's a core patch for this (at a duplicate issue) which is RTBC. It would be great if people could try it out: #496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion.

> avert this issue and alleviate this notoriously difficult to identify bug until the core

Nobody seems to have done any research into what happens when you just let the link options be an empty array. Presumably some of the flag's settings are not taken into account. Does the link even work properly?

I really feel this fix is not a fix -- it's like keeping driving a car when a wheel's fallen off.

> There is no guarantee when or if it will be fixed in D7. It's quite the norm for modules to code around issues or limitations in the core and contrib APIs.

@rwohleb: I haven't checked, but have any of the commenters on this issue gone to help with the core issue? Have you?

Status: Closed (works as designed) » Needs review

Status: Needs review » Needs work

The last submitted patch, 132: flag-broken-1925922-131-1.patch, failed testing.

vinoth.3v’s picture

Status: Needs work » Closed (works as designed)

I opened this issue, and now I am closing this as project maintainer claims that it is nothing to do with this module. further conversation should be discussed on core bug mentioned in above comments.
Please don't reopen it. If you want the fix please apply the simple patch #131
Or work with core team to fix the core issue of drupal 7.

marcoka’s picture

had that to owhen updating mdoules with drush up. did a drush cc all. and ran drush udate db again. error was gone.

rwohleb’s picture

Status: Closed (works as designed) » Active

The core issue that supposedly is to blame for all of this is now marked RTBC. However, I still get the error no matter how many registry_rebuilds and cache flushes I try. Looking at their solution, I believe I understand why. It now prevents the incomplete implements record from being written to the database, but if there is already corruption it can still get in the way of things. Thus the core patch that is RTBC will prevent cache_bootstrap from getting corrupted due to being written in too early a bootstrap phase. However, it does nothing for corruption from other sources. This could be due to other misbehaving contrib modules, actual data corruption at the database layer, or even an issue with a database backup that is being restored. In these types of situations, you want your system to fail gracefully so that recovery is much more likely. The only thing holding anyone back in this situation is a simple type casting being added to the flag module so that it avoids a fatal error. Even if the core patch fixed this, most of us cringe at the thought of running a patched core in production.

@vinoth.3v, with the state of the RTBC core patch and my findings, I unfortunately find myself having to open this ticket again.

@joachim, If you review my messages in this thread you will find them to be moderately patient and constructive. On the other hand, you are coming across very differently, which I can't believe is not your intent, so please excuse any frustrations. And before you ask, yes I've been following that core issue, just as I do with many others. Please, for the love of [god|satan|godzilla], help us make the flag module a better citizen in the contrib space by avoiding an insanely simple fatal error edge case.

Look, no one expects the flag module to fix the corrupted data, but we don't want it to blow up in our faces either. The great thing about the solution I proposed in the updated issue summary is that it's correct no matter how core is or is not fixed. It's not a hack, or anything that will cause issues down the road. It's just a typecasting like is already happening in other places in the flag module. Such a small change will help anyone afflicted with this terrible situation, and you win by not having to read my posts any more. Win, win!

rwohleb’s picture

@joachim, in reference to your question in comment #143 about the link array being empty: yes, I looked into this. If I recall correctly, it can cause the flag link to be malformed due to missing data. However, that's only true as long as the initial error state persists, so most won't get a chance to notice since they have bigger fish to fry. That would be a much preferable state of affairs than a fatal error. Is that what you were looking for?

Renee S’s picture

I agree, rwohleb: error-checking is development best practice. Not doing it because it shouldn't need to be done -- even though it clearly does -- is frustrating: we live in a world that isn't perfect, and designers of system have to account for the context in which they operate. Clearly that context is producing data corruption, and regardless of why, a good program checks and accounts for things like that.

rwohleb’s picture

Status: Active » Needs review
FileSize
736 bytes

Here is a super simple patch that skips any type casting and just does an is_array() test. In my tests this lets a registry rebuild continue unaffected after corruption from that core bug or another source.

Status: Needs review » Needs work

The last submitted patch, 151: flag-sanity-check-1925922-151.patch, failed testing.

rwohleb’s picture

Status: Needs work » Needs review
FileSize
580 bytes

It would help if I uploaded the correct patch file.

kevinquillen’s picture

So back to one of the original patches? I can confirm, this fixed all the issues I was having with Flag that related to this.

It's worth applying until it is addressed in core.

rwohleb’s picture

@kevinquillen, yep, since I still see the issue even with the RTBC core patch installed. Thanks for reminding me about the slightly better patch in #1925922-76: Unsupported operand types in factory_by_row() in flag_flag.inc, whose author should get the proper credit.

brandy.brown’s picture

patch in 76 worked for me.

Kamlendra’s picture

Hi All,

I also faced this issue:
Fatal error: Unsupported operand types in /var/www/my_website/sites/all/modules/flag/includes/flag/flag_flag.inc on line 132

This issue occurred when I made some changes in a form alter, and then after doing code review for a while and searching internet to solve this issue. I realized that this was caused because of the following wrong code that I used:

   form_set_value($form['name'], $name, &$form_state);

The above code put my site into fatal error and wsod after I cleard the cache.
Replacing above code with the following one:

  form_set_value($form['name'], $name, $form_state);

removed & before $form_state solved the problem.

Hope this helps someone who ran into this issue inadvertently.

joachim’s picture

Status: Needs review » Postponed (maintainer needs more info)

A fix was committed to core at #496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion 4 days ago, so it would be great see people who have had this problem try core 7.x, or the next core release, and see whether it fixes it for them.

fortis’s picture

I will try on next release

nerdcore’s picture

I am using Drupal 7.41 and still experiencing problems described in this issue.

I am using Flag 7.x-3.7.

One item which makes me curious is the use of a slash character ('/') in one of the four entries in database column flag.options. Are there characters which should be avoided in Flag titles by chance?

I also experienced an issue where the site caused a PHP Fatal Error as described in the issue title here (flag/flag_flag.inc on line 132 of the 7.x-3.7 version of Flag) but only on the site when the "www" prefix was used; Accessing the same site using the non-www base FQDN worked fine. As this was occuring on a production site all of a sudden, I have simply forced canonicalization of the URL to the non-www variant, but I have no idea why one hostname would cause this error and another would not. :(

mgifford’s picture

Status: Postponed (maintainer needs more info) » Active

This is still coming up as a problem with us.

I am curious if this might be tied to clearing caches much like https://www.drupal.org/node/2421609#comment-10806228

"view result and the rendered output are cached separately, if the rendered output is cleared before the view output is cleared then" ... bad shit happens.

epringi’s picture

This is the third time in the last six months this has cropped up, only on production, cannot replicate. It's intermittent and 'fixes itself', so difficult to troubleshoot.

The error I'm getting:
PHP Fatal error: Unsupported operand types in /data/www/.../sites/all/modules/flag/includes/flag/flag_flag.inc on line 132

The lines of code:

[130]    // Populate the options with the defaults.
[131]    $options = (array) unserialize($row->options);
[132]    $options += $flag->options();

Obviously somehow $flag->options() is returning something other than an array. I simply wrapped $options += $flag->options(); in an if statement containing an is_array() check to avoid this error. Hope this helps others having to deal with this.

nerdcore’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 162: flag-array-check-1925922-162.patch, failed testing.

Island Usurper’s picture

I finally tracked down this problem in my case. Warning: I am a special flower, though my situation may not be unique.

My site has filefield_paths and amazons3-2.0. The version of amazons3 is important because it instructs you to patch Drupal. I believe the problem arises because filefield_paths.module is loaded before flag.module, which prevents flag_hook_info() from being loaded. How, you ask? filefield_paths.module uses file_scan_directory() to load some extra files outside of any functions. file_scan_directory() calls file_stream_wrapper_uri_normalize() which amazons3 patches to include a call to drupal_alter(). And of course, drupal_alter() calls module_implements() before all of the modules have been loaded, even though the new check for the bootstrap phase passes.

I'm going to file an issue against filefield_paths, because that code clearly belongs in filefield_paths_init().

jrobison’s picture

Here's the latest patch I've added on my local for 7.x-3.9.

empesan’s picture

Ralkeon’s picture

In my case, #168 doesn't apply cause the path to the file was wrong. Here is the correct one.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

This problem caused a fatal error for a site I was working on today, under PHP 7.4.6. Like the issue summary, this site also has the Rules module along with Flag. The error message we got:

Error: Unsupported operand types in flag_flag::factory_by_row() (line 132 of [...]/sites/all/modules/flag/includes/flag/flag_flag.inc).

I was able to fix the problem by applying the patch in #169. I looked at the patch, and cannot imagine it would cause any problems to commit it, so I am going to go ahead and mark it RTBC.

poker10’s picture

I had the same issue and patch #169 worked to fix it.

alexh’s picture

Same issue and error message as in #170 - just upgraded PHP from 7.2 to 7.4.28 and clear the cache to get his fatal error.
Applying the patch #169 solved the problem.
It would be good to commit this.

joachim’s picture

  1. +++ b/includes/flag/flag_flag.inc
    @@ -129,7 +129,11 @@ class flag_flag {
    +    // Make sure flag->options is an array first.
    

    Comment is wrong.

  2. +++ b/includes/flag/flag_flag.inc
    @@ -129,7 +129,11 @@ class flag_flag {
    +    if (is_array($options) && is_array($flag->options())) {
    

    We know $options is an array, as we just cast it to one.