Save DataTable metadata in records if added during archiving. #14671
Conversation
diosmosis
added
RFC
There was a problem hiding this comment.
Looks good in general. Left a comment though. Not sure what happens for aggregated data tables @diosmosis ? During archiving this would be probably handled in the archiver, when mergeSubtables or so is called, it would I think keep the metadata of the first table, which should be fine I suppose. At least for our use case. 👍
core/DataTable.php
Outdated
| * This allows us to save datatable metadata in archive data. | ||
| */ | ||
| const ID_ARCHIVED_METADATA_ROW = -3; | ||
| const LABEL_ARCHIVED_METADATA_ROW = -3; |
There was a problem hiding this comment.
could it be an issue when -3 is actually a value? Like usually so far we don't archive the totals row for example so there it was maybe not an issue? Similar to ranking query wondering should that maybe be like "_metadatarow__archive"?
There was a problem hiding this comment.
👍 makes sense.
This would also be a problem w/ the summary row as well... actually, rows are associated by ID, not label, so there might be some issues in the future, but perhaps not now. I'll use the better label regardless.
There was a problem hiding this comment.
@diosmosis be good to make the change 👍 BTW: You maybe also want to move it up next to the other LABEL* constants
| // gather metadata before filters are called, so their metadata is not stored in serialized form | ||
| $metadata = $this->getAllTableMetadata(); | ||
| foreach ($metadata as $key => $value) { | ||
| if (!is_scalar($value) && !is_string($value)) { |
There was a problem hiding this comment.
@diosmosis quickly looked through some archivers and dataArrays and it seems like there's usually no metadata on the dataTable so that should be fine. Haven't checked search engine plugin or so. I wondered if we really want to persist by default automatically all metadata or whether we should maybe require to specify whether some/which metadata should be persisted or not? Not sure if it would be always wanted to persist all information. I guess we could also just leave it like this, and eventually change it later when need comes up to whitelist specific metadata.
There was a problem hiding this comment.
I could add a 'metadata_to_serialize' metadata, though maybe that's just too much meta...
There was a problem hiding this comment.
I think it's not needed for now. Initially thought it was a good idea, but then realising it's probably over-engineering in the sense that we seem to not have a use case for it just yet and we can always add it later. I reckon be good to keep it simple for now.
Couldn't find any table that sets metadata during the archiving in core and our plugins. Don't think there would be any third party plugins out there that do it, and if so, it likely doesn't even cause an issue. I suppose if you set metadata, maybe it's what people expect that it gets persisted 👍
@tsteur what do you think about this change? This would allow me to set a 'is_imported_from_google' metadata on a table before archiving.