Define basic conventions for converting things into classes

Hm. I understand the examples are just examples, but all of them are targeting more complex subsystems than the simple things I thought of. I.e., cache, db, etc are complex things that are statically tracking state and stuff.

Our risk is to make zero progress, because of epic debates about a "perfect pluggable architecture". The current functions are not pluggable currently, so why even think of making them pluggable now?

As a very first baby step, I really thought of straightforward conversion of procedural functions into (static) class methods only, and moving the class into a separate include file (if not the case already). That gains us auto-loading of stuff that's currently loaded but not used, and also clarifies what stuff we actually have.

OLD: includes/common.inc:

function flood_register_event($name, $window = 3600, $identifier = NULL) {
function flood_clear_event($name, $identifier = NULL) {
function flood_is_allowed($name, $threshold, $window = 3600, $identifier = NULL) {

NEW: includes/flood.inc:

class Flood {
  public static function register_event($name, $window = 3600, $identifier = NULL) {
  public static function clear_event($name, $identifier = NULL) {
  public static function is_allowed($name, $threshold, $window = 3600, $identifier = NULL) {
}

Note: Flood might be a bad example. Based on what I see, Flood should be a module.

@sun, could you elaborate on why you said Flood should be a module? I am confused about what is now procedural code in common.inc being moved into static class methods vis-a-vis a module. Is it because of possible response to hook_* calls?

I do not believe that simply replacing the first underscore in a function with a class name and static method (#2) is a good idea. While that does get us some lazy-loading, that is *all* it gets us. I agree that we should not immediately shoot for perfect, but we should also not shoot for "trivial hack". :-) That does not offer any sort of testability improvement at all; it also means we cannot separate out code to different files at all, since a class must be entirely contained within a single file.

At minimum, we should use an instantiated object with at least a trivial factory. That will help us see how and where something is actually used. If the object tracks very little data of its own, then the actual performance cost of creating the object should be fairly small. The existence of an object is not that much more than any other ZVal, I believe.

subscribe

sub.

I agree with Crell in #4. We should try to capture the most basic logic in a factory class that can be lazy loaded and then the rest in methods that makes sense.

The following is not strictly related to this issue; but if some sort of pluggability is needed for the "thing", one could do as I proposed in #1252486: Low level UUID API in core. There I basically have an interface, a simple factory class and some very basic implementations. The factory class is of course possible to lazy-load and it decides on the implementation to use by using variable_get('thing_class'). It's basically similar to the cache thing, but used a lazy-loadable factory class instead of procedural functions.

I know that this might be jumping ahead of a proper plugin system, but at least it's a simple pattern we potentially could move forward on now.

just my two 2 cents...

Edit: wrong issue link

sub

A few thoughts....

1. Shouldn't we use namespaces from the start? If D8 is PHP5.3+ and we want to avoid collision with other libraries/scripts we could start using them now. Class files would look like:
   

   <?php
   namespace Drupal\core\cache;
   
   Class default implements cacheInterface {
     ...
   }
   

2. Factories are a must because they can support a number of features we have, like automated testing. A simple example is the cache system. While your site is running memcache you could test the database cache. Wouldn't that be nice? I'm personally a fan of factories being static methods on a class. They can be autoloaded.

   The one thing that comes to mind is the DX (within the Drupal community who is not typically accustomed to classes and namespaces). For example, I could just imagine the difficulty of someone implementing:

   <?php
   use \Drupal\core\cache\factory as cache;
   
   ...
   
   function foo() {
     ...
     $data = cache::load($bin)->get($cid);
   }
   

   From a DX standpoint it might be easier to have something that makes this easier while still autoloading:

   <?php
   function foo() {
     ...
     $data = cache($bin)->get($cid);
   }
   

   In this example cache() is in a file (maybe something akin to common.inc that includes the use statement and wraps the static method calls to make it an easier transition.

3. I really disagree with #2. When we convert element lets turn them into classes the right way the first time. If I see something like this it will be a bad day:
   

   
   Class title {
     static function get() {
       return title::set();
     }
   
     static function set($title) {
       ...
     }
   }
   

Fun note: it seems the \ in namespaces does not play nice with our php filer.

I agree with mfer that factories are in general a good thing, as are interfaces. However, I disagree that static methods make good factories in general. Their only advantage is that they will self-lazy-load. However, they are then still not truly separating the "creation behavior" from the "being behavior" (the factory from the object being created).

I'm fine in general with a utility function to wrap a factory call if it is easier. The database layer uses that a lot. However, we should still emphasize dependency injection over direct calls for all of the testability reasons we've mentioned before.

sub (interested in helping with the novice issues once this is agreed).

ps. if there are any other pressing novice issues (or more complicated issues which you can guide on) send me a message.

subscr

Sub.

Sub. This will need to concern Field API at some point.

Sub

sub

Drupal's kind of bad at failing hard when you give functions bad data. I'm willing to be convinced otherwise, but that's my impression :). Would it make sense to do things like the following in order to do more automatic sanity checking?

class ValidationException extends Exception {}

class Language {
  public function __construct(array $data) {
    $this->data = $data;
    $this->validate();
  }

  public function save() {
    $this->validate();
    // ... bla bla save the language
  }

  public function validate() {
    if (empty($this->data['name'])) {
      throw new ValidationException("Incorrect data passed to Language.");
    }
  }
}

function do_cool_thing_with_language(Language $language) {
  // this should fail if the argument is not a language
}

// This would fail because the argument isn't a Language object
do_cool_thing_with_language('asdf');

// This is going to fail because the language object doesn't have a 'name'
$other_language = new Language(array(
  'name' => NULL,
  'language' => 'fr',
  // ... etc
));

do_cool_thing_with_language($other_language);

I consider this better DX because you find out exactly why you've done something wrong very quickly. Thoughts?

tarmstrong: Our coding standards already encourage interface-based type specification, but never, ever class-based. (It's more flexible that way.)

catch: I meant a static factory method on the parent class. Eg,

Entity::load('node', $nid); // returns instance of Node, which extends Entity

That's what I was arguing against.

If you're already using a separate class anyway for the factory (which is good), I don't see why you wouldn't just instantiate it.

sub

sub

sub

In response to something Crell said in #10 about factories as static methods on classes...

There are a number of benefits for classes as static methods on classes:

1. The already mentioned autoloading (which is very useful in and of itself).
2. A class can be cleanly overridden without hacking core. A super simple way would be to register another autoloader ahead of the core one in the spl autoloader queue so the alternate implementation of the class gets loaded instead of the core one. This could lead towards something more maintainable on those projects that hack core.
3. A class implementation that autoloads is easier to push to a php extension. You don't have to hack core for it to work.

Most of these don't hit the default use cases but they are definitely nice to have for people pushing the limits of Drupal.

Vs. a purely function factory, everything in #26 is true. However, all of those benefits apply to an object factory as well, which is what I was arguing for over a static class factory in most cases.

sub

From #10:

I'm fine in general with a utility function to wrap a factory call if it is easier. The database layer uses that a lot.

Those are convenience wrappers only, and until/unless we have a proper services locator system I am OK with them. That doesn't mean they contain any real functionality.

I think we're saying the same thing, with the caveat that the utility function should be dumb wrappers around an underlying OO factory (one of the other two). That way if you really do need to use such a system before that file loads it's plenty easy to do, and the code weight of loading a bunch of utility functions in common.inc or bootstrap.inc is minimal.

subscribe

subscribe

subscribe

subscribe

sub.

Utility functions, as described in #31 + #32, seem to be the simplest approach to this problem, with the minimum of overhead for updates and reasonable DX. Its close to what occurs in a lot of procedural stuff in Drupal anyway, and easy to switch implementations on.

The static method is possibly slower than the others anyway, as I discovered while hacking away at http://drupal.org/node/1177246.

That probably solves the Factory Method part of this discussion. What other aspects are there to this issue?

sub.

Just my $.02 from one of the newest kids on the block...

Have we decided exactly what core's purpose is? Versus the modules' purposes?

I was reading the online documentation for Apache the other day, and something written there struck me as profound. I can't seem to find it at the moment, but it went something like this.

The entire purpose of Apache (or any web server) is to accept requests, rewrite them just a bit, then locate and serve the requested resource. --paraphrased from Apache.org documentation

That's it!

Shouldn't we be trying to bring that same simplicity to Drupal's core? For both the UX and the DX of the experience, Drupal has an enormous community of expertise from the oldest tyros to the youngest prodigies.

I believe in the KISS principle... Keep It Simple, Silly.

Bringing that degree of simplicity to the user and developer experience is going to require a much greater degree of complexity inside the black box. But, IMO, that is what Drupal needs, I'm coming to believe. I also believe that to pursue this is a task of years and enormous labor from the community as a whole. At the end, though, I see a product that defines CMS, industry-wide, that can build a basic blog site for the newest of us, as well as a globe-spanning enterprise system, as well as anything in between.

I don't see this as an issue for Drupal 8.x, sorry to say. More in line with Drupal 10.x or 11.x. I'm thinking that on the day Drupal 8.x is released, the basic feature set of Drupal 9.x will be fixed, and discussions for what is needed for Drupal 10.x begin.

Sorry about that... the $0.02 grew to $0.25. Inflation occurs everywhere. :)

Paul

A proposed aphorism:

The principle of Conservation of Complexity states:
Complexity cannot be created or destroyed. It can only be hidden or exposed.
-- Unknown

My vote goes toward hiding the complexity from the outside world, and exposing the simplicity to the user and developer community.

@Paul_Simard This is a well-worn discussion but this issue is not the right place for it. Catch has a good write up on the current state with a number of links you should investigate if you want to contribute.

http://ca.tchpole.net/node/4

Sub

@xtfer Thanks for the link. Very interesting read. No more mention here. Sorry.

Crell #27

Vs. a purely function factory, everything in #26 is true. However, all of those benefits apply to an object factory as well, which is what I was arguing for over a static class factory in most cases.

I completely support Crell on this static vs function vs object thing.
Static methods are so tempting for many developers, that we will/would quickly see plenty of static methods replacing regular functions, for obscure reasons. I would call this the "class as namespace" anti-pattern, and I would really like Drupal to avoid falling into this trap.

So, for my taste, either regular functions, or class instances, whatever does make the most sense in a particular situation. (both for factories, and other things).

Class names with module prefix.
I think we need a naming scheme for classes that reflects Drupal's separation of code into modules.
Unlike some typical PHP frameworks and packages out there, a Drupal codebase is not an arbitrarily nested directory structure, so let's not try to mimick those frameworks' naming patterns 1:1.
Within one module, there is usually not too deep of a nested filesystem structure, so it would be ok if all class files live within the same folder.

We should aim for something that is safe against nameclashes, and where the class name reveals which module it belongs to.
Still, the class name should reveal that this thing is a class, not a function. So a little bit of ucfirst / camelcase should be there.

So far in my own modules I used a CamelCase class name, prefixed by the (lowercase) module name with underscores.
So, "view" would become "views_View". More general, some_module_SomeClass.
In some cases I add lowercased suffixes, to add descriptive attributes to the name, but that's a matter of taste.

I think having the original module name being part of the class name is preferable to a distorted, camel-cased version of the module name.

I would appreciate if we can agree on something in this direction.

catch #31:

<?php
$db = new Db('query');
$db->query($sql)->fetchField();
?>

[..]
You have to instantiate at least two classes to get anything done.

Having to create the factory in-place as a "new Db('query')" kind of defeats the purpose, imo.

So, "drupal_db('query')", instead of "new Db('query')".
I think this only becomes interesting if either
- you get the $db object injected "from above", or
- you retrieve it from a retrieval function such as "$db = drupal_db()", or
- you retrieve it from a retrieval object, which again was injected "from above", or
- your example code is actually the "from above" level, and you are going to pass the $db object around.

I think, as long as we don't have any fancy injection mechanics in place, we should stick with the regular-function factory / retrieval function.

Re #47, we're going to have a PSR-0-compatible autoloader in core in Drupal 8. I don't know yet how we'll leverage that in modules, but I am hopeful that we will be able to use that somehow rather than Crazy_Long_Class_Names_That_Violate_Coding_Standards.

Anyone following here should also be following there,
#1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch]

I think we're done with this issue.

This is definitely fixed for D8.

For D7, I think the current naming conventions for classes are quite lame.
We cannot count on namespaces, but I would be happy for something where the module name is suggested as a prefix.
(as supported in xautoload)

So, should we have a better convention for D7? Or just stick with the rather stupid one we have now?

I was thinking more of new classes, mostly in contrib.
But I guess it's fine, just leave the convention as it is, and contrib can just ignore the convention and prefix things if they prefer to do so.