Revealed via #1373142: Use the Testing profile. Speed up testbot by 50%

Problem

  • Renaming a node type bundle causes notices on manage fields page (and fields on the bundle are likely not moved).

Reproduce

  1. drush -y si minimal; drush -y en field_ui
  2. visit 'admin/structure/types/add', create content type
  3. edit content type from 2., just change the machine name and save
  4. visit the manage fields page for the content type from 2.
  5. FAIL, notice as in the test.

Attached patch has a test that does that, and throws the test exception.

Looks like some random dependency on something that amounts to a missing field_cache_clear() call.

Related issues

#1079966: Notice: Undefined index: [custom_content_type_machine_name] in _field_ui_bundle_admin_path(), line 309 in field_ui.module

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Status: Active » Needs review
catch’s picture

Status: Needs review » Needs work

The last submitted patch, node_field_ui_bug.patch, failed testing.

sun’s picture

Component: node system » field system
Status: Needs work » Needs review
FileSize
1.15 KB

Thanks @catch for digging out that issue -- it's pretty obvious now.

sun’s picture

Status: Needs review » Needs work

Apparently, #4 does not fix the bug, so we're back to square one.

However, clearing the entity info cache seems like a good idea either way, since bundles are also cached there, no?

Anonymous’s picture

this bug is caused by stale info from _node_types_build().

when we clear the entity info cache, the next call to entity_get_info calls into module's hook_entity_info implementations.

node_entity_info calls in to node_type_get_name, which returns stale info right back into our freshly rebuilt entity info. awesomeness.

sun’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
3.01 KB

First of all, attached patch exposes the bug.

sun’s picture

I guess this would be the most simple solution, err, stop-gap fix.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

yes, i think we need to go with the simple fix for now, then open follow up patches or link to existing cleanups in the entity system.

this is a simple patch, with a test, i think its RTBC.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I've opened #1376884: Use configuration for entity types ;)

This looks good to me, comes with a nice test, and fixes a bug exposed by testing cleanup. Committed/pushed to 8.x.

Anonymous’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.4 KB

and here's the D7 version.

Anonymous’s picture

also, look at the lists of likely related issues in #686938-61: Undefined index: admin in _field_ui_bundle_admin_path() ... ewwww.

pjcdawkins’s picture

The patch in #11 works for me, but I haven't tested the test.

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

Go!

droplet’s picture

#11: rename-bundle-1375452-11.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, rename-bundle-1375452-11.patch, failed testing.

oriol_e9g’s picture

Rerolled. Two patches, one with only the tests and other with all.

oriol_e9g’s picture

Status: Needs work » Needs review
oriol_e9g’s picture

Same with small whitespaces fix.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

thanks oriol_e9g, lets see what webchick thinks.

webchick’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

Confirmed that the patch clears up the bug.

The one thing I'm not as crazy about though is the test. While I can confirm that without the rest of the patch, the test fails on its own, I'm not able to reproduce it in the minimal profile by following the steps (Create a content type, a vocabulary, and a term reference field). Versus I can clearly trigger the bug with beejeebus's steps in the OP, and "rename bundle" seems like a worthy thing to have a test for.

The comments in the test also don't clear things up for me as to why this actually triggers anything because they say:

+    // Create Basic page and Article node types.
...
+    // Create a vocabulary named "Tags".

...and? :) What's the punchline? What's the condition are we checking for?

So if we could either re-work the tests to be more along the lines of beejeebus's steps in #1, *or* more clearly articulate in these tests what we're testing here (with an assertion or similar) that would really help.

sun’s picture

There is a FieldUIManageFieldsTestCase::testRenameBundle() test already.

However, the test only reveals that the code throws an exception with the entity_info_cache_clear() from #7.

The lines outlined in #21 are technically irrelevant for this issue - they merely make FieldUIManageFieldsTestCase work with the Testing profile (the master patch revealed this bug).

That said, we could improve testRenameBundle() to test the expected results a bit more explicitly. All it currently does is:

    $this->drupalPost('admin/structure/types/manage/' . $this->hyphen_type, $edit, t('Save content type'));

    $this->drupalGet('admin/structure/types/manage/' . $hyphen_type2 . '/fields');

...without any assertions after each of those requests.

At the very least, we could assert confirmation messages, and response status codes. That said, the drupalGet() always yields a positive 200 response, because the menu router automatically falls back to admin/structure/types, in case the %node_type cannot be found - so the test should verify that the GET actually shows the Manage fields page of the renamed node type.

catch’s picture

Title: Renaming a content type bundle causes notices on manage fields page » Renaming a content type bundle causes notices on manage fields page (test improvements)
Category: bug » task
Priority: Major » Normal
Issue tags: +Needs backport to D7

This is just test fixes now, downgrading status.

Tanrry868’s picture

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

#19: rename-bundle-137452-19.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, rename-bundle-137452-19.patch, failed testing.

drupalycious’s picture

Status: Needs work » Needs review

#19: rename-bundle-137452-19.patch queued for re-testing.

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

The last submitted patch, rename-bundle-137452-19.patch, failed testing.

drupalycious’s picture

Hello,

as the patch failed testing, is someone interested to modify it?

Thanks

swentel’s picture

Status: Needs work » Needs review
FileSize
1.05 KB

so the test should verify that the GET actually shows the Manage fields page of the renamed node type.

This should do it.

swentel’s picture

FileSize
1.09 KB

Actually, this is even better.

yched’s picture

Works for me, but would need a phpdoc block on ManageFieldsTest::manageFieldsPage() then.

swentel’s picture

FileSize
1.17 KB
yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, works for me.

webchick’s picture

#32: 1375452-32.patch queued for re-testing.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Oops, sorry. That was me sucking at Git. :P

Committed and pushed to 8.x. Thanks!

Moving to 7.x for backport.

lias’s picture

Has this been backported? I've got the latest drupal version, 7.21, installed and I'm still getting these errors.

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.37 KB
2.77 KB

Backported #8 and #32 to D7.

corbin’s picture

Issue tags: -Needs backport to D7

#37: 1375452-37.patch queued for re-testing.
oops, sorry.
The patch doesn't seem to work for kickstart;
"doesn't seem" because maybe I made another error :-(

saurabh tiwari’s picture

Issue tags: +Needs backport to D7

#37: 1375452-37.patch queued for re-testing.

dags’s picture

Status: Needs review » Needs work

#37 did not fix the issue for me using Drupal 7.23. I encountered the error after enabling the core Blog module and then disabling it. Patch #7 in #1898110: Notice: Undefined index: blog provides a work around for the issue.

dags’s picture

Issue summary: View changes

Updated issue summary.

kopeboy’s picture

Issue summary: View changes

Still getting a lot of these errors at Reports > Field list, on Drupal 7.31, related to:

Blog
Forum
Comment
Commerce Node Checkout
(all uninstalled apart from Comment)
and a content type I renamed.

Any update?
Still needs code fixes or should I just re-enable and re-uninstall the modules?
What about the content type (which now I don't remember which one was?)

Thanks

jessZ’s picture

Running 7.31 with get this error. If enable forum and comments, errors refer to comments instead.
Notice: Undefined index: forum in _field_ui_bundle_admin_path() (line 325 of /home/reimagi8/public_html/new/modules/field_ui/field_ui.module).
I also get this error with a number pf content types that were deleted in drupal 5 and 6. (this site has survived upgrade from 4.7 > 7.3

After i deleted alot of unneeded field types that showed up correctly i was able to run database update with nor errors but up until then i was getting a dependency warning when trying to update the database.

  • catch committed 231ad0d on 8.3.x
    Issue #1375452 by sun, beejeebus: Fixed Renaming a content type bundle...
  • webchick committed 1149786 on 8.3.x
    Issue #1375452 follow-up by swentel: Test improvements for renaming...

  • catch committed 231ad0d on 8.3.x
    Issue #1375452 by sun, beejeebus: Fixed Renaming a content type bundle...
  • webchick committed 1149786 on 8.3.x
    Issue #1375452 follow-up by swentel: Test improvements for renaming...

  • catch committed 231ad0d on 8.4.x
    Issue #1375452 by sun, beejeebus: Fixed Renaming a content type bundle...
  • webchick committed 1149786 on 8.4.x
    Issue #1375452 follow-up by swentel: Test improvements for renaming...

  • catch committed 231ad0d on 8.4.x
    Issue #1375452 by sun, beejeebus: Fixed Renaming a content type bundle...
  • webchick committed 1149786 on 8.4.x
    Issue #1375452 follow-up by swentel: Test improvements for renaming...

  • catch committed 231ad0d on 9.1.x
    Issue #1375452 by sun, beejeebus: Fixed Renaming a content type bundle...
  • webchick committed 1149786 on 9.1.x
    Issue #1375452 follow-up by swentel: Test improvements for renaming...