Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:109687
User-Agent: Cyrus-JMAP/3.1.7-1131-g3221b37-fmstable-20200415v1
Mime-Version: 1.0
Message-ID: <11f35e10-5bee-4b28-b784-69f7f5f2f11e@www.fastmail.com>
In-Reply-To: 
 <CAEPJdunDbMY6AdTRTfQ8VEnwY-ey6jTG2sKFHz=kqoykvSc89A@mail.gmail.com>
References: 
 <CAEPJdunSr8PkBtSLO8g_JMLzM4cFVnXsQSEMFmu2RN25XT=grw@mail.gmail.com>
 <CAEPJdumF8GaAqLj+=gNi-dMP1GYSvk_DYJuy3gG0N5J1vSXRqQ@mail.gmail.com>
 <16620f3a-1eb6-437e-aa6f-05e5be2aa713@www.fastmail.com>
 <CAEPJdun15CfEjxBsbMbnv8XWHnOvdZ0fnB-E1aYsrPbRWavVTw@mail.gmail.com>
 <231390ee-ff9c-4f7b-918a-4f87a676613e@www.fastmail.com>
 <CAEPJdunDbMY6AdTRTfQ8VEnwY-ey6jTG2sKFHz=kqoykvSc89A@mail.gmail.com>
Date: Thu, 16 Apr 2020 09:28:49 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] Re: [RFC] Attributes v2
From: larry@garfieldtech.com ("Larry Garfield")

On Thu, Apr 16, 2020, at 1:46 AM, Benjamin Eberlei wrote:

> > > > 3. I see the most common case for attributes being getting the object
> > > > version.  With the reflection API as currently described, I see two
> > > > shortcomings.
> > > >
> > > > A) I can't tell if an attribute has a valid object or not before
> > trying to
> > > > access it, which would presumably fail spectacularly.  I believe we
> > need a
> > > > way to know if getObject() is going to return a valid value before
> > trying
> > > > to call it.  I think this is a hard-requirement.
> > > >
> > > > B) Related, as is getting all attributes as objects looks to be rather
> > > > clunky.
> > > >
> > > > $attribute_objectgs =
> > array_filter(array_map(function(ReflectionAttribute
> > > > $r) {
> > > >   if ($r->getObject()) { // Needs something better here.
> > > >     return $r->getObject();
> > > >   }
> > > > }, $obj->getAttributes()));
> > > >
> > > > That's gross. :-)  Can "get all the attributes that can be formed into
> > > > objects" be its own operation?  $obj->getAttributeObjects() or some
> > such,
> > > > that skips over non-instantiable attributes and instantiates the rest?
> > > >
> > >
> > > I don't see A.) what would you do when the object instantiation fails?
> > You
> > > would throw an exception I presume, let the engine throw the regular
> > > TypeError, ArgumentError, Error if class not exists that everyone is
> > > already familiar with.
> > >
> > > For B.) I believe you are extrapolating based on your own use case.
> > Working
> > > with Reflection is usually a lot of boilerplate, I don't believe we need
> > to
> > > have a one liner here.
> >
> > It depends on the annotation, I suppose.  If I'm requesting a specific
> > annotation by name, presumably I know if it is supposed to have an
> > associated class.  If it's supposed to but it's missing, that's a legit
> > class-not-found exception/error.
> >
> > However, I'm thinking of cases where code is integrating with a 3rd party
> > optionally, through an annotation.  In that case it's a fair question of
> > whether the class will be defined or not based on whether some other
> > library is present.
> >
> > Similarly, if a bit of code is requesting all attributes (as above) rather
> > than just specific ones by name, it wouldn't know if a given attribute is
> > supposed to be defined or not; as written, class-less attributes are
> > supported.
> >
> > I suppose the workaround would be class_exists($r->getName()).  Weird but
> > I guess works?  It would have to be documented as a thing you should do,
> > though, which implies to me that it could be made cleaner.
> >
> > That reflection is usually clunky today (true) is to me not a compelling
> > argument that it shouldn't be made less clunky. :-)
> >
> 
> You are not safe from these problems when using Doctrine Annotations either
> (missing library or class does not exist) and it fails exactly the same way
> as trying to instantiate something that doesn't exist.
> 
> I also realized why IS_INSTANCEOF is not the default, because it needs to
> resolve all attributes to classes to perform the check. This triggers
> autoloading *all* attributes of the reflected declaration (even the ones
> not requested), so we felt it should not be the default.

Ah, valid.  I suppose that's an unavoidable result of allowing non-class-mapped attributes, which means anyone building on it is stuck doing their own class_exists() check for everything.

Sad panda.  (Still very +1 on the RFC, just sad panda about these details.)

--Larry Garfield