NOVICE PROJECT:
Of what was discussed below, what remains is that in core/lib/Drupal/Core/Queue/DatabaseQueue.php the __construct() method has documentation saying it is constructing a factory object. It isn't. That one line of documentation should be fixed to say it is constructing a DatabaseQueue object.
--- Original report ---
In conjunction with
#1815988: UUID class names violate naming standard
today, I noticed that the base UUID class in core/lib/Drupal/Component/Uuid/Uuid.php is documented as "Factory class for UUIDs".
It isn't actually a factory class though.
http://en.wikipedia.org/wiki/Factory_(software_concept)
A factory is an object for creating other objects. This class isn't one -- it is a class that instantiates one of several back-end implementations, and then provides a UUID service by calling methods on the class it instantiated. If it was a factory, it would just return the instantiated implementation class.
So, that documentation needs to be changed. We should not be calling things "factory" if they are not really factory classes.
It would be helpful if someone could look through the rest of core for classes documented as "factory" and make sure they are really factory classes. This may not be the only one. Could be a good Novice project...
Comment | File | Size | Author |
---|---|---|---|
#10 | drupal-DatabaseQueue-constructor-documentation-1924818-10.patch | 507 bytes | hanoii |
Comments
Comment #1
marvin_B8 CreditAttribution: marvin_B8 commentedList from all Factory classes:
Wrong use of factory :
other:
i hope this list is complete.
Comment #2
jhodgdonThanks for making this list!
If you go to api.drupal.org, switch to Drupal 8, and search for "factory" (no quotes), then filter by type "class" (no quotes), I think there are some additional classes there... although maybe you got them all? Not sure, your list is slightly difficult to scan as it is not in alphabetical order.
Comment #3
mtiftHere is an alphabetical list of all classes whose namespace contains the word "Drupal" and that have the word "factory" in the description:
While I did not look at all of them, all of the ones I examined seemed to fit the definition of a "factory." Because all except Uuid have the word "factory" in them leads me to believe that most of these are, indeed, factories.
Also, I attached a patch for the DatabaseQueue class.
Comment #4
mtiftComment #5
abghosh82 CreditAttribution: abghosh82 commentedThe comment "Constructs this factory object." seems sensible, rather than "Constructs a connection object.". As it is a DatabaseQueue object with queue name and connection object, rather than solely a connection object.
Comment #6
socketwench CreditAttribution: socketwench commentedNovice issue cleanup.
Comment #7
jhodgdonHm, old issue (more than a year old). Since this was filed, the Uuid class has changed and it no longer says it is a factory.
The problem with DatabaseQueue still exists. The patch in #3 is not correct though -- it's not constructing a connection object (or a factory), but actually it's constructing a DatabaseQueue object.
We should just make a new patch. That does sound like a Novice issue?
Comment #8
hanoiiI just picked this issue on DrupalCon, will try to help on it.
Comment #9
hanoiiAlthough Uuid doesn't have the issue any more, I went to see where that class was actually being used. Apparently it's just a container for an isValid method that's being used within test classes only. Only noting it as it's something I just found.
EDIT: And if anyone can add information on its real purpose there and it was added as a class method that will help me understand more on the logic behind it, that'd be great.
Comment #10
hanoiiAttached is a patch that fixed DatabaseQueue constructor documentation. I went over other classes as well, most of what are mentioned in the issue, and there's not a consistent documentation for constructors. Some mentions the whole namespace+class, some just says it creates the CLASSNAME object, others says nothing. I wonder if a constructor should have an actual documentation besides the parameter description, maybe only if the constructor does something specific, but otherwise maybe is just worth to agree on a common documentation scheme for classes?
Comment #11
quartsize CreditAttribution: quartsize commentedI wonder also, but this patch does indeed solve the misleading comment problem.
Comment #12
hanoiiI found a related issue that mentions some policy on documentation that's some how related to this, I still wonder if constructors need documenting at all but so I don't add more discussions to that issue will create another one.
Comment #13
hanoiiActually I just found the issue where the constructor documentation concerns has been raised already, so I added my opinion there as well.
Comment #14
jhodgdonThanks again! Committed to 8.x. Let's continue that related discussion on the other issue.