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...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marvin_B8’s picture

List from all Factory classes:

WidgetFactory  ok
QueryFactory oK
FormatterFactory ok
ViewExecutableFactory ok
TypedDataFactory ok
MetadataFactory ok
CacheFactory ok
TempStoreFactory ok 
DefaultFactory ok
ReflectionFactory ok
ConfigContextFactory ok
QueueDatabaseFactory ok
QueueFactory ok
KeyValueMemoryFactory ok
KeyValueFactory ok
KeyValueExpirableFactory ok
KeyValueDatabaseFactory  ok
KeyValueDatabaseExpirableFactory ok
TypedConfigElementFactory ok
BootstrapConfigStorageFactory ok
UUID wrong
DatabaseQueue wrong
ConfigFactory wrong

Wrong use of factory :

UUID wrong
DatabaseQueue wrong
ConfigFactory wrong

other:

RegisterKernelListenersPass no comments

i hope this list is complete.

jhodgdon’s picture

Thanks 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.

mtift’s picture

FileSize
485 bytes

Here is an alphabetical list of all classes whose namespace contains the word "Drupal" and that have the word "factory" in the description:

BootstrapConfigStorageFactory
CacheFactory
ConfigContextFactory
ConfigFactory
DefaultFactory
FactoryInterface
FormatterFactory
KeyValueDatabaseExpirableFactory
KeyValueDatabaseFactory
KeyValueExpirableFactory
KeyValueFactory
KeyValueMemoryFactory
MetadataFactory
QueryFactory
QueueDatabaseFactory
QueueFactory
ReflectionFactory
TypedConfigElementFactory
TypedDataFactory
Uuid
ViewExecutableFactory
WidgetFactory

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.

mtift’s picture

Status: Active » Needs review
abghosh82’s picture

The 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.

socketwench’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Novice

Novice issue cleanup.

jhodgdon’s picture

Issue summary: View changes
Issue tags: +Novice

Hm, 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?

hanoii’s picture

I just picked this issue on DrupalCon, will try to help on it.

hanoii’s picture

Although 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.

hanoii’s picture

Status: Needs work » Needs review
FileSize
507 bytes

Attached 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?

quartsize’s picture

Status: Needs review » Reviewed & tested by the community

I wonder also, but this patch does indeed solve the misleading comment problem.

hanoii’s picture

I 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.

hanoii’s picture

Actually I just found the issue where the constructor documentation concerns has been raised already, so I added my opinion there as well.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 8.x. Let's continue that related discussion on the other issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.