Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 49 60 +11
===========================================
Files 1 2 +1
Lines 159 179 +20
===========================================
+ Hits 159 179 +20 ☔ View full report in Codecov by Sentry. |
794322c to
ac6d770
Compare
|
lgtm |
db75879 to
af726e4
Compare
27502d4 to
a05140e
Compare
| $buffer = $this->defaultObjectExport($value, $processed, $indentation); | ||
| } | ||
|
|
||
| return $class . ' Object #' . spl_object_id($value) . ' (' . $buffer . ')'; |
There was a problem hiding this comment.
one more thing I just noticed.
the $class . ' Object #' prefix will only be present in "root objects". in custom object exporters this $class . ' Object #' format needs to be replicated, even if the custom exporter delegates export back to the built-in one.
There was a problem hiding this comment.
Should we deal with that in Exporter::exportObject() or should we trust implementors of custom object exporters to do the right thing?
There was a problem hiding this comment.
I don't see a way how Exporter::exportObject() could do that in a way which would even work when custom exporters would delegate exporting of some stuff back to the exporter object.
Maybe we just need a test-case so we can see whether it works already or what a custom ObjectExporter needs todo to get it right
There was a problem hiding this comment.
Maybe the problem would be less theoretic if we try to implement a real world exporter
Maybe @BladeMF could try his use-case..?
There was a problem hiding this comment.
I like that ... and would rather not make a release without this feature having been validated through real world use cases.
There was a problem hiding this comment.
Sorry, missed it in April. Let me have a look.
There was a problem hiding this comment.
Right, we can try some pseudo-code first and see where that takes us, OK?
So, if I was to write a Doctrine entity exporter the case might be something like the following. A typical entity object graph might look like this (the notation is (<class-name>)#<instance-name> for classes and then <property-type> $<property-name>: <property-value>, the below is taken from a recent object but simplified):
(Folder)#1 {
int $id: 4,
string $name: "Folder 1",
array|(ArrayAccess&Countable&Traversable) $visibleToEmployees: (PersistentCollection)#2 {
(listing the items below)
(Employee)#3 { ... }
(Employee)#4 { ... }
(Employee)#5 { ... }
}
array|(ArrayAccess&Countable&Traversable) $editableByEmployees: (PersistentCollection)#6 {
_listing the items below_
(Employee)#3 { ... } (this instance appears a second time)
(Employee)#6 { ... }
(Employee)#7 { ... }
}
}
so the exporter code would be something like this:
public function export(mixed $object, Exporter $defaultExporter, int $indentLevel): string
{
// we assume we already know it's an entity
$metadata = $this->metadataFactory->getMetadata($object);
$indent = str_repeat(" ", $indent);
$indentPlus1 = str_repeat(" ", $indent + 1);
$lines = [];
foreach($metadata->properties as $property) {
$value = $property->getValue($object);
if($value instanceof Collection) {
// it's a Doctrine collection
$objectLines = [];
foreach($value as $item){
// see coment below about calling the default exporter
$objectLines [] = $indentPlus1. $defaultExporter->export($value, $indent + 1);
}
$lines[] = $indent . $defaultExporter->formatExport($value, implode("\n". $indent, $objectLines));
continue;
}
// Use the defaut export, it will call me back to check if I support it if is an entity
// I could check it here and do a shortcut, but this is way more cleaner:
// if it's an entity, I get the list of properties from the repository (as opposed to all class properties)
// and then handle the collections.
// There also could be another exporter with greater priority than me,
// so it's not really legal for me to jump the line
$lines[] = $defaultExporter->export($value, $indent + 1);
}
return "\n".implode("\n". $indent, $lines).
}the formatExport function might look something as silly as:
function formatExport(object $object, string $buffer): string
{
return $object::class . ' Object #' . spl_object_id($object) . ' (' . $buffer . ')';
}It appears that a formatExport function is needed. I think the purpose of the custom exporter is just to export a specific class, nothing more, nothing less. It should defer anything else to the default exporter. I am not even sure the formatExport function is in the Exporter, it looks like a separate formatter class may be needed, but I won't die on that hill. I also kind of dislike the need for me to deal with the indent before the closing brace ), because it's mine-1, but I don't really know that, because the default formatter/exporter determines that, if that makes sense.
If the above (a bit messy) code is not convincing I could try and write one for real.
I am in the middle of moving to the seaside for 3 months, but I will try and think more about that code. It may very well be the case that I need to write one real.
This reverts commit d0d99db.
Follows sebastianbergmann/phpunit#5773 (comment).