-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Treat some ArrayAccess
classes like array
or stdClass
#144
base: main
Are you sure you want to change the base?
Conversation
ArrayAccess
classes like array
or stdClass
46e6070
to
6c9020c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems very good to me 👍
Would it possible to add a test with ArrayObject
and/or one of the MongoDB class ? 🤔
I like the idea of supporting ArrayAccess. However, It may be useful to handle all array access classes without having to define all possible classes by doing the following thing :
Then we could do something like : class Foo
{
public string $foo = 'foo';
public string $bar = 'bar';
}
class Bar extends \ArrayObject
{
public string $foo;
// ...
}
$automapper->map(new Foo(), Bar::class));
var_dump($bar->foo); // foo
var_dump($bar['bar']); // bar This will map the There will be no need to defined a list of class (however we could implement this feature as an opt-in so we don't break existing code that use ArrayAccess) I believe this should work with Document of MongoDB extension also |
That would provide a partial list of supported properties as source or target, which something not supported currently, that raise a lot of questions. I'm using a list of supported classes because some Model classes could implementation ArrayAccess in order to serve their actual properties. |
I know that some apis or others lib provide this design in order to support extra attributes / data into an object, as an example symfony provide that for their Event
We could avoid using those classes by using the ignore property of the MapTo / MapFrom attribute so don't think it's a problem |
I'm afraid it won't work. $extractor = new Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor();
$properties = $extractor->getProperties(\ArrayObject::class);
// array:4 [
// 0 => "arrayCopy"
// 1 => "flags"
// 2 => "iterator"
// 3 => "iteratorClass"
// ] Same for $extractor = new Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor();
$properties = $extractor->getProperties(\MongoDB\BSON\Document::class);
// array:4 [
// 0 => "iterator"
// ] We don't want them to conflict with actual values provided using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated: Mapping an ArrayAccess
to ArrayAccess|array|stdClass
is not supported (same as array to array)
if (!$metadata->isTargetUserDefined()) { | ||
return []; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why there is it condition. It was introduced by 1486a85#diff-538038bd35959095e72795c2a102193df6a1ee0c17b27dd7f129e23d8f2464ecR111-R113
This prevent instanciating new ArrayObject
.
Given this RFC, which challenges |
Sorry for the late answer, i was struggling on what's best here and i think i have a proper solution. We still want in the future to allow a user to write something like this :
And the mapper would map foo and extra properties if available. However, like you said, in some cases we don't want to extract some properties, although there is the possibility to ignore them, it's may be difficult to add when it's an external class. The overall goal would be to allow to set a strategy for a source and target like this:
Strategies are cumulable (except for having array + dynamic), so someone can tell this class should be class + array strategies, or only array strategy, etc ... We would also provide sane defaults:
I don't want to force you to write all of this, are you ok if we merge this after the next release and rework it to fit our vision ? |
This is not a final implementation, just the way to expose the issue.
When querying MongoDB, documents are returned as
MongoDB\BSON\Document
. This generic class implementsArrayAccess
, it can contain any field as anarray
orstdClass
.In order to support denormalization from MongoDB Documents to class, I need to modify the code of the library to add exception on top of
array
andstdClass
.Instead of being specific to MongoDB classes or wide to all
ArrayAccess
implementation, a config is necessary to set the list of classes to handle as arrays.Why would this be interesting when you can convert documents to
array
? Because withMongoDB\BSON\Document
, field values are read only when they are accessed.