pending change might not point to a GedcomRecord

[BertKoor]

fix #5090: on the forum it was reported that accepting or rejecting pending changes makes the error in the home/my page block go away. Thus these cases where $record == null can be safely ignored.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 35.53%. Comparing base (5c3ef34) to head (86fe322).

Files with missing lines	Patch %	Lines
app/Module/ReviewChangesModule.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #5310   +/-   ##
=========================================
  Coverage     35.53%   35.53%           
- Complexity    11126    11127    +1     
=========================================
  Files          1159     1159           
  Lines         47843    47843           
=========================================
  Hits          17002    17002           
  Misses        30841    30841           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fisharebest]

Did you manage to reproduce this error?

This seems to be a situation that should not occur.

I would prefer to fix the original error, not just hide the consequences.

[BertKoor]

I tried, but without any result. Cause could be a glitch I sometimes get when a new spouse has a link to the family, but no reference back was created. And the cause of that is unknown.

[BertKoor]

May I ask why this PR did not simply get accepted?
There are hundreds, if not thousands checks for instanceof in the webtrees codebase where logically the result of that check could never be false. Here imho it should have been in place since day one.
Remember that the moderator accepting/rejecting changes is likely not the user which encountered something odd when edits were made. So handling it gracefully is imho the best you can do.

[fisharebest]

There are hundreds, if not thousands checks for instanceof in the webtrees codebase where logically the result of that check could never be false.

I don't think this is true. We use phpstan to analyse the code, which would flag these cases.

The other cases are where we genuinely expect the value to be null.

This one is different. The null case should never happen. If it does, there is a bug somewhere else.

$record = Registry::gedcomRecordFactory()->make($change->xref, $tree, $change->new_gedcom ?: $change->old_gedcom);

AFAICT, this will only return NULL when new_gedcom and old_gedcom are both NULL.

This should never happen. If it does, there is a bug in the edit/change system. I don't want to hide/suppress a bug in an important part of the code.

Maybe something like this?

if ($record === null) {
  throw new Exception('Invalid change record for ' . $change->xref. ' - see issue #5090');
}

[fisharebest]

Actually, let me investigate this some more.

[fisharebest]

Create a record (e.g. a source) and delete it immediately.

From the pending changes page, the accept/reject links don't work (although the accept/reject all links do).

This seems releated - it uses similar logic. In those cases, there is an "if instanceof" check.

This check is present because the change may have been accepted in a different window, and the record may no longer exist.

But it also means that the code fails silently in this case.

I'm looking at the caching layer, to see if that's the cause of the null values...

[fisharebest]

It might also be relevant that webtrees does not check/enforce uniqueness for different types of XREF. You can import a GEDCOM containing the same XREF on two different types of record (e.g. 0 @X1@ SOUR and 0 @X1@ REPO), and webtrees still works.

[fisharebest]

From the pending changes page, the accept/reject links don't work (although the accept/reject all links do).

Fixed in f24e5c6

[BertKoor]

That's a change, but I'm not (yet) convinced it addresses the root problem. With a rotten change record present, the detail list of all pending changes will still throw an exception (or it showed a blank list with spinner? Don't remember). So a moderator only has a choice to blindly reject or accept all to get out of that, which is not good.

Wouldn't an import set the central xref counter higher than highest xref present in the imported data? It must be a new record. Or a change which could not fetch the old gedcom.

Question is, how are corrupted change records created. We only know for sure that both old & new are not instanceof GedcomRecord. No idea what the xref is, whether it is valid...

v2.2.5 introduced a regex check on gedcom data validity. I tried with 2.2.4 to create an empty Media Object: click save on an empty form. I think it changed a newline on the individual (last event was marked as changed) but no trace of the Media object in pending changes. I have not tried the same routine on adding a new partner. Also it could be a module which makes Place/Location a first class citizen. I don't use any of those.

Food for thought?

[fisharebest]

The factory method that creates records takes 2 or 4 parameters.

• tree + xref
• (optional) current + pending GEDCOM

If the record is logically deleted, we get null.
If we pass in two GEDCOM versions (from a change record), we get a record.

HOWEVER, we cache the result.

So, it is possible to

1. fetch the record using just an XREF. This gives null, and caches it.
2. fetch the record using data from wt_change. This should give a record (which we expect), but actually returns null from the cache.

This would explain our issue. I just need to set up a reproducable test case.

If i can, then the solution may be to only cache results where we only supply the tree/XREF.

[fisharebest]

I tried with 2.2.4 to create an empty Media Object: click save on an empty form.

This is probably a bad type of record for this test. New Media objects (or editing media files on media objects) get accepted immediately, so that the databae and filesystem stay consistent.

[fisharebest]

I have managed to reproduce this error (although this is unlikely to be the same method as the original issue.)

On the home page, add two blocks - favourites and pending-changes (in that order).

Create a new record, as a pending addition.

Add it to the favorites block.

Delete the record.

View the home page.

Now, the call from the favourites block returns NULL, as this record is deleted. This value is stored in the cache.

But the pending change block will try to create a record from the old/new versions. This should give a record, but gets NULL from the cache.

[BertKoor]

So you found a way to create a change record with status pending and no old nor new payload.

How does the list of all pending changes show it? Can it be accepted or rejected, one way or another?

[fisharebest]

I think the issue is that we have two separate pieces of logic in the same function.

1. Create a record from (a) the "accepted" value in the database and (b) the latest pending change. This is used throughout almost all of webtrees. The result is cached. This is for more than performance. It means that requests for the same XREF will return the exact same object, so we can use logic like $record1 === $record2.

2. Create a record from two specific revisions of the GEDCOM. This is used for the pending changes block.

Both use factory::make() - which is cached.

factory::make() calls factory::new() to create the record - which is not cached.

I think the solution might be for the pending changes block (and any other similar code??) to call factory::new() directly.

This might need some type-detection (INDI/FAM/etc.) so that we can create the right sort of record.