SAVED_UPDATED is not always SAVED_UPDATED

Ok this is really crappy. So hook_menu_update() only executes if you change either the menu title or description fields. Any other modules that add form items to the menu edit form are SOL about the hooks getting executed if only those fields are changed.

The reasoning for this patch is menu_save() uses db_merge() to update the menu title and description. If modules have altered the menu edit form and the users changes those fields but not title or description, the db_merge() query will return 0 since no records were updated, and not SAVED_NEW or SAVED_UPDATED. This prevents the hook_menu_update() from being invoked.

This affects the XML sitemap module for D7. Anyone up for a quick review?

This is a broken hook, so marking as major.

#4: 801894-hook-menu-update-skipped-D7.patch queued for re-testing.

This affects a bit more than just menu_save(). It would be wrong to change drupal_write_record(), but we want to adjust similar code accordingly.

Good catch sun! Looks good that the same bug is fixed in similar places, although it doesn't look like they affect hooks like the menu chunk does.

Needs tests to make sure we don't break this again.

Looking at the patch, I don't understand why it needs to change any form submit handlers? It seems like anywhere this issue occurs, it should always be fixed inside the corresponding API function. And in some cases - for example, filter_format_save() - I know that the API function already handles this situation, so we shouldn't need to change anything in the filter.module here.

We should also add documentation that menu_save() now has a return value...

Otherwise, looks great.

David, that almost translates into directly changing drupal_write_record() to always return one of both constants.

I'm not sure whether API functions should be changed in a way that callers don't have to care. If we want to do this, then we don't have to change gazillions of API save functions, but can fix drupal_write_record() once for all instead.

@Dave: Do you still tackle this one?

I'd like to hear feedback from base system maintainers on how to move forward here.

Yeah I can work on re-rolling this tomorrow.

I looked into this a bit and I actually don't think the bug exists anymore.

• drupal_write_record() was fixed a long time ago (#626790: drupal_write_record returns FALSE for valid update queries was committed to Drupal 7 last December), and there is a test proving that it works as expected.
• db_merge() also appears to have been fixed, though more recently (I think in #844186: Clarify merge queries, but I'm not sure). If so, the attached test should pass.

If I'm right, there isn't much to do here anymore except clean up any code that previously existed to work around this bug.

Also, it's a somewhat disturbing fact that db_merge() returns either MergeQuery::STATUS_INSERT or MergeQuery::STATUS_UPDATE, but we have lots of code in core that assumes its return value should be checked against SAVED_NEW or SAVED_UPDATED instead (i.e., relying on the fact that those constants happen to have the same numerical values)...

Well shiver me timbers, I'm glad to see it got fixed a little more upstream in the code recently. We should maybe add tests that will assert that STATUS_INSERT == SAVED_NEW and STATUS_UPDATE == SAVED_UPDATED then.

Why on earth do we have two new constants that completely duplicate existing ones we already had for years?

Good question. My guess is because the database layer was written to be independent of the rest of Drupal, so it only uses constants which it defines itself? That is kind of annoying in a situation like this, but getting rid of that duplication sounds too late for D7.

However, there is no reason we can't modify all callers to use the correct constants and check them appropriately; that is not an API change at all (especially given that they are numerically equivalent).

The attached patch should take care of all necessary cleanup. Also, I think we've determined that there is no longer a serious bug here, so I'm downgrading the priority.

No longer applies.