class TestClass {
  private $value;

  public function __construct($value) {
    $this->value = $value;
  }

  public function test() {
    static $s;
    if (empty($s)) {
      $s = $this->value;
    }
    return $s;
  }

}


$t1 = new TestClass("test1");
echo $t1->test();

$t2 = new TestClass("test2");
echo $t2->test();

... will echo:

test1
test1

Several parts of the DB:TNG make the opposite assumption (including the DatabaseConnection->schema() function)... and that simply breaks having multiple connections to different databases.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
9.45 KB

Here is a patch.

I also found an issue in SelectQuery->addExpression():

   public function addExpression($expression, $alias = NULL, $arguments = array()) {
-    static $alaises = array();

That typo probably makes the alias uniqueness check in addExpression() fails. We should add a test for this.

Damien Tournoud’s picture

Updated patch, fix a typo in DatabaseConnection->insert().

Dries’s picture

This looks correct to me but it would be great to get sign-off from Crell.

Crell’s picture

Status: Needs review » Needs work

Hrm. I suppose it makes sense the way PHP is doing it. Blargh. Good catch, Damien.

Moving all of those to a member variable looks like the right approach. However:

- All member variables should be defined at the top of the class definition so that they're easy to find in one place.

- All member variables should be protected, never ever private. private makes them inaccessible to child classes, and child classes are how we allow driver-specific handling where necessary.

- Member variables should use camelCase, the same as methods.

- Include the @var directive for member variable docblocks. (API module doesn't currently parse them but it doesn't handle classes at all. The @var is part of phpDoc, which we should be moving toward anyway.)

- I'd suggest $preparedStatements rather than just $statements. Also mergeClass rather than merge_class_type, etc. (I'm not super picky on what those get called, though.)

- The SelectQuery::$alias property looks like it's only used for expression aliases, as field aliases use a different algorithm that doesn't require a member variable. It should therefore be named SelectQuery::$expressionAliases, or something like that.

I believe as a side-effect of this patch, aliases will also be sequenced per-query, not per-connection. I think that's probably a good thing. I'm just mentioning it here for completeness.

How hard would it be to include a test case for this? I suppose we should, although it's not immediately obvious how without having both a MySQL and Postgres database available at the time the test is run.

Damien Tournoud’s picture

@Crell: those are not supposed to be member variables, but merely statics. That's why I put them near their function (and not at the top), and why I chose to mark them private, not protected. I'm not sure it makes sense to give access to those internal variables to child classes.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
9.67 KB

Renamed the variables and added @var parameters.

I stand that those should remain private and stay where they are (very near their consumer method).

As far as testing is concerned: #195416: Table prefixes should be per database connection will make simpletest uses different connections (one prefixed, the other not prefixed), so simpletest will completely break if this does not work.

Crell’s picture

Every other class in the entirety of the database layer (which is every class in Drupal but one) follows the common convention of member vars first. These *are* member vars, even if they're only used by one method. I am also actively avoiding private on anything, for the simple reason that it is unnecessarily limiting to future extension. It's rude to other developers who may need to do things you can't think of yet. :-)

I need to talk to the Coding Standards group about making that an official coding standard, actually...

Damien Tournoud’s picture

Status: Needs review » Needs work

We just discussed this with Crell on the IRC, and could agree only on one thing: that it makes sense to make expressionAliases protected.

I still believe that having a "no private" policy simply makes no sense and will encourage hacks and bad design practices.

Because I'm already fed up with this, I'll leave this for anyone else to design.

Crell’s picture

Assigned: Unassigned » Crell

I'll try to get to this tonight or this weekend sometime.

Dries’s picture

I'm not getting the "no private" policy either. If these variables are meant to be static, than they should actually be private to prevent child classes from messing with them. It is a feature, not a bug.

Plus, opcode optimizers can optimize private member fields away. They can't optimize protected or public member fields away because they have to assume that the class might be extended dynamically.

Crell’s picture

The problem with private variables is that they are effectively final. You can't do anything to do them from a child class, period. That unnecessarily limits your extensibility. Just because you can't think of a use for something now doesn't mean you won't be able to later. That sort of boxing yourself in too early is easily the biggest potential pitfall of OO design.

I opted to make nothing private for that reason. I can't predict that no one will ever need to access a given method or property differently than I expect them to. With procedural code, you always have the fallback of "well just call the function". I've had to do that with "private" functions in CCK before to get certain functionality that was impossible any other way. With OOP, you don't necessarily have that escape hatch. The closest you get is "well just subclass it", which works in some cases. If you have a private method or property, though, that escape hatch is welded shut.

I'd not heard of opcode caches caring about private properties before. Without seeing benchmarks that sounds like a premature micro-optimization, though, especially given how crappy our performance is in other places.

Crell’s picture

Status: Needs work » Needs review
FileSize
12.23 KB

Cleaned up coding style. Fixed a few comments to not say that things are "statically cached" anymore since they're not. Changed how expression aliases are generated to match the logic in addFields(), which eliminates the need for a cache in the first place. Adds a unit test (yay!) for multiple expressions to make sure the aliases are generated properly. Tastes great, less filling.

chx’s picture

FileSize
12.1 KB

There is nothing new in this patch just shuffled code around a bit, I use $class = $this->deleteClass; new $class($this, $table, $options) instead of just ; new $this->deleteClass($this, $table, $options). It's murky enough, there is a lengthy code comment explaining the two possible behaviours of this line: this will create a class named $class and pass the arguments into the constructor, instead of calling a function named $class with the arguments listed and then creating using the return value as the class name. I can't say one is more logical than the other but PHP behaves this way without ugly parentheses so that's good.

pwolanin’s picture

had a fatal error and some problems w/ comments. Fixed those and ran the database tests. All the database tests pass (577 passes, 0 fails, and 0 exceptions).

pwolanin’s picture

FileSize
12.05 KB

oops - didn't diff all the files.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed, tested and approved.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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