It is not possible to index content after disabling the forum module. The comment module is trying to load a field when indexing and this field does not exist. This is the line:
$comment_statistics = $entities[$record->entity_id]->get($field_name);
To reproduce it you also need at least active comment field on the given entity type - eg
Forum comments + article comments -> disable forum -> forum comment field is removed -> view a node with a comment from the forum comment field (since removed).
To reproduce this behavior you need to:
Skip fields that entity does not have.
None.
None.
None.
Original issue title: After uninstalling a node type module, nodes are not loadable, breaking search indexing and admin/content/node
[Drupal 7] If by some reason node can't be loaded (for example, node's module disabled, but the row in {node} table still exists), then site indexing process interrupts on this node.
function _node_index_node($node) {
$node = node_load($node->nid);
// Save the changed time of the most recent indexed node, for the search
// results half-life calculation.
variable_set('node_cron_last', $node->changed);
// Render the node.
$build = node_view($node, 'search_index');
......
If node_load() returns FALSE, then this value (FALSE) will be transferred to node_view() and will result in an error "EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7633 of /var/www/creativepro/includes/common.inc)" and interrupting site indexing.
We need to add check for successful node_load() to _node_index_node() function:
function _node_index_node($node) {
$nid = $node->nid;
if(!($node = node_load($nid))) {
watchdog('node', t('Can\'t load node @nid during site indexing.'), array('@nid' => $nid), WATCHDOG_WARNING);
return;
}
// Save the changed time of the most recent indexed node, for the search
// results half-life calculation.
variable_set('node_cron_last', $node->changed);
// Render the node.
$build = node_view($node, 'search_index');
....
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | comment-disabled-field-1966448.25.pass_.patch | 2.86 KB | larowlan |
| #25 | comment-disabled-field-1966448.25.fail_.patch | 2.13 KB | larowlan |
| #25 | interdiff.txt | 2.13 KB | larowlan |
| #20 | Screen shot 2013-11-14 at 1.07.17 PM.png | 166.81 KB | marthinal |
| #16 | interdiff-1966448-13-16.txt | 1.62 KB | marthinal |
Comments
Comment #1
codeyourdream commentedPatch.
Comment #2
TonyK commentedThis issue relates only to search module and I think it should have major priority because it makes a part of nodes not indexable.
Comment #3
TonyK commentedTo reproduce disable Forum module (provided the site has nodes of type forum) and try to re-index nodes.
Comment #4
catchThere's already a critical issue to address behaviour like this: #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed.
Downgrading to normal - if you disable forum module while having forum nodes still on your site many more things can break than this, which is what the above issue is for.
Also needs fixing in 8.x first then backporting.
Comment #5
TonyK commented@catch
#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed seem to be a big problem with Drupal architecture. Fixing it (whenever it happens) will likely introduce non-backward compatible changes to Drupal core. Are there really any chances that it will ever get to D7?
Comment #6
jhodgdonThe code is quite different in Drupal 8 now.
For one thing, it's NodeSearch::updateIndex(), and it is doing basically a query to get the node IDs from the node table, and then calling
I have no idea what will happen if loadMultiple() is unable to load the node for whatever reason... probably an exception? Is it even possible to get into that situation in Drupal 8? Can we make a test that reproduces this behavior?
Regarding disabling/uninstalling modules... If you uninstall the Forum module, I just looked at the code and it does *not* remove any nodes when you uninstall the module -- it removes some taxonomy vocabs and some fields related to forums, but it will leave the nodes there.
So we can probably make a test out of this: install/enable Forum and Search, make a forum term and a couple of forum nodes, plus then a couple of other nodes, uninstall forum, and then run cron to index the site and see what happens (which nodes, if any, are indexed, and does cron finish, and is there an error message?).
You could try this manually and if it makes errors, then write a SimpleTest for it. Then we could either try to fix the actual bug (make it so that loadMultiple does not die when a node-defining module is removed), or we could try to work around it in search (do a try/catch on the loadMultiple), or who knows.
What happens now if you add Forum nodes, uninstall Forum, and then go to a node/### page for one of those forum nodes? What should happen?
Comment #7
berdirloadMultiple() should simply return an empty array and make this a no-op and just skip the foreach. Assuming that doesn't break anything later on, we should be fine.
Comment #8
jhodgdonOK then. I think we should make a test for D8 as outlined in comment #6. Assuming it passes without any code change (as #7 suggests it will), we can then commit the test and move on to Drupal 7. Any takers?
Comment #9
marthinal commentedI was reproducing the bug creating forum content and others(page,article...). Then disabling Forum module.
If you try to index the content, it will be not possible. The problem seems to be at this line (comment module):
$comment_statistics = $entities[$record->entity_id]->get($field_name);This is because $field_name == comment_forum.
I have fixed the problem with this patch. Now I can reindex the content and everything seems ok.
Let's take a look at the test result.
I don't know if we can fix this problem from other place. But this get() method break the index process.
Comment #10
berdirUse $entities[$record->entity_id]->hasField($field_name). getField() isn't bundle specific, so that doesn't work like that. If anything, you'd have to use getInstance().
Comment #11
marthinal commentedUsing hasField() there were problems so I decided to use getInstance as you proposed.
Comment #12
berdirWhat kind of problems? The function was added for exactly this use case, if there's a problem, we should fix it.
Comment #13
marthinal commented@Berdir upppss Probably I made something wrong using hasField. I'm sorry. Attached again using hasField.
Comment #15
jhodgdonThe last patch doesn't look like it got tested, so resetting status... Actually I'm not seeing that last file in Recent Files? Um... maybe upload it again?
Comment #16
marthinal commentedlet's try again, sure.
Comment #17
berdirThis isn't really search specific problem, it's the generic comment entity load hook. So anything that would attempt to load those entities would then fail.
Not 100% sure what exactly is going on, but it seems that the data there shouldn't exist anymore if the field doesn't, so it would never have to attempt to set it.
Maybe @larowlan can comment on this.
Comment #18
larowlanBerdir pointed me at this issue, and yeah not sure why there would be data for a field_id if that field no longer existed. Can you update the issue summary with the field_id you're seeing and the entity type?
Thanks
Comment #19
marthinal commentedComment #20
marthinal commentedScreenshot attached.
Comment #21
jhodgdonOK, let's step back here.
I just went to simplytest.me, and spun up a Drupal 8 alpha 4 site, which isn't -dev but has the advantage of being quick to set up. Did the following:
- Installed using standard install profile in English.
- Enabled Forum (Search/Comment were enabled)
- Added a Forum node. Added a comment to it.
- Went back to the modules uninstall page and uninstalled forum. Verified it is not enabled on the Extend page.
- Clicked on "Content" (admin/content/node). Instead of the Content page, I got this error:
InvalidArgumentException: Field comment_forum is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField() (line 398 of /home/s98ab04be11a0bfa/www/core/lib/Drupal/Core/Entity/ContentEntityBase.php).
So this is definitely a problem in Drupal Core, independent of search.module, at least as of alpha8. I didn't get as far as trying to index the site, but if you can't even get to the admin/content/node page, there isn't much point in trying to index your site for search.
I think it would be easy to make a test that reproduced these steps, and then we could verify that the patch would fix the behavior.
At this point, I'm thinking that for 8.x it is not a search module problem. Seems like field.system?
Comment #22
berdirIt's the field system that causes the error, but comment.module is the actual cause here, which does not use it correctly.
Comment #23
larowlanworking on tests
Comment #24
marthinal commented@larowlan I was working on this test last weekend and today a little bit because I had some problems to reproduce using a minimal installation. This test reproduce the error and #16 fix it. The only problem, other tests fail because there's a conflict enabling and disabling modules. So I know you are working on it and maybe this test is useful to finish your test or... I don't know at what point you are.
Comment #25
larowlanthanks @marthinal
I added this to ForumUninstall test, it is already concerned with field behaviour when disabling forum module.
To reproduce it you also need at least active comment field on the given entity type - eg
Forum comments + article comments -> disable forum -> forum comment field is removed -> view a node with a comment from the forum comment field (since removed).
Comment #26
larowlanNo such thing as comment-field in D7
Comment #27
jhodgdonIf we're not marking this as 'needs backport', then we need to file a new issue with the original report from D7, because search indexing breaks due to this same problem in D7.
Comment #28
andypostSuppose D7 case is different, patch is awesome
Comment #29
jhodgdonActually, this isn't quite right. The test-only patch did not fail the test. Therefore, it is not a valid test of this bug.
And let's go ahead and leave the backport tag. Although the patch will be different, and the underlying cause is a bit different, the bug is pretty much the same.
Comment #30
larowlanThe tests are still running?
Comment #31
jhodgdonOh sorry. I saw the green and didn't look at the details. The D6 issue page display was a lot easier to scan. :(
Anyway, let's leave this at "needs review" until the tests have failed/passed appropriately.
Comment #33
larowlanFail test failed.... still waiting on pass
Comment #34
jhodgdonExcellent! I am in agreement that this is RTBC assuming the other test passes, in which case the test bot will set it back to needs work.
Comment #35
alexpottSo the issue title and summary are now completely out of whack as it refers to both the D8 and D7 issue. It's hard to know what to do here. But the actual issue summary and title need to relate to the rtbced patch.
Also since this this patch is not backportable removing that tag. Not sure what to do now about the D7 issue - ideally we would have created a new issue when the new d8 bug was found.
Comment #36
alexpottReading #25 makes me realise my issue summary is not quite correct - updated steps to reproduce.
Comment #37
alexpott25: comment-disabled-field-1966448.25.pass_.patch queued for re-testing.
Comment #38
jhodgdonI added a new issue for D7:
#2153999: After disabling Forum module, cannot do search indexing
It references this one as 'related'.
Comment #39
alexpottCommitted 4a80834 and pushed to 8.x. Thanks!
And @jhogdon thanks for opening the other issue.