EVENT_PRE_REFERENCE does not support altering metadata in event subscriber

[swirtSJW]

Current Behavior

If I create an event subscriber for LifeCycle::EVENT_PRE_REFERENCE and in that subscriber I alter metadata for the dataset, it does not persist. It gets overwritten because the reference uses the metadata from before the event is dispatched, rather than after any subscribers have run.

When referenceMetadata() fires it extracts the $metadata, then dispatches the event. Then my event subsciber alters the metadata inside $data. However then on line 361 it essentially saves the metadata that it set aside at the beginning, which is unchanged. So all my metadata maniplulation never got saved.

Expected Behavior

If I alter metadata as part of an event subscriber to LifeCycle::EVENT_PRE_REFERENCE, the alteration should persist and get saved with the referenced entity.

Steps To Reproduce

1. Create an eventsubscriber to LifeCycle::EVENT_PRE_REFERENCE
2. Alter the dataset node's metadata in the subscriber.
3. Inspect the saved dataset and notice that the alteration did not get saved.
4. Curse the PHP gods for their abject cruelty.

Relevant log output (optional)

Anything else?

This will be testable on PDC.

[swirtSJW]

This PR worked, in that it allowed the "EVENT_PRE_REFERENCE" to be used like a Drupal hook to make alterations to the metadata if needed, prior to the reference being created. Ultimately it did not solve the data dictionary problem on PDC so I do not end up needing it.

I think this is still a useful addition that allows others to modify data, as compared to the existing pattern that is reactive only (we can react to the event but can not alter the data being referenced).
There is one caveat that someone could alter the metadata in a bad way because the changes do not get re-validated. So there is some risk, but there is similar risk in any implementation of any traditional hook too.

[paul-m]

LifeCycle::referenceMetadata() wants to get the metadata, and then it wants to turn metadata into UUID references instead of a URI.

So first, its docblock is incorrect.

Second, MetastoreItemInterface::getMetadata() is helpfully documented in the docblock as "Protected." Thanks much.

Third, we end up at Data::getMetadata(), which is also helpfully documented as "Protected." But for some reason it has to call Data::fix() before returning the metadata, which seems like a code smell.

So we have to ask: Is it important to call Data::fix() before we use the referencer to reference our metadata? In other words, do we need to fix() before we call the PRE event?

Data::fix() calls fixDataType() and saveRawMetadata(), which both seem kludgey, but maybe due to our circuitous reference/dereference/lifecycle might be necessary.

The only listener for the PRE event in DKAN core is DatastoreSubscriber::onPreReference(), which checks if the datastore should be reimported, based on config. It does a compare of values within the metadata. So if Data::fix() is necessary, it might be so this reimport comparison can occur.

[swirtSJW]

@paul-m that is an interesting trip down the rabbit hole. I like all the cleanup you have done in that #4641
I'm not clear on the implications to the PR for this issue (#4628). Are you suggesting that I should add a call to Data::fix() within referenceMetadata()?