If we use protected properties, we presume we have thought of every possible use case ahead of the time. As #802514: changeConnection from hook_query_alter already proves this is simply not the case. This patch removes protected modifier from the database and adds an underscore instead. The underscore convention is well understood and we can add more documentation stating that if you mess with these properties, your loss. As someone else said "you lose your warranty" We should not worry that now you can mess up your site -- you have unnumbered ways to mess up already (didnt Views recently change the roles array of the global user object even if there was no view on the page?) instead we should worry about losing flexibility. I talked to Moshe and sdboyer and they -- curiously -- agreed.

This is not an API change, this is an API addition. OK, all three contrib modules implementing DBTNG drivers need a change. Big deal. (I wrote one. I will live.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

FileSize
165.14 KB

Sorry, that did not include install.inc and pager.inc

Status: Needs review » Needs work

The last submitted patch, dont_be_overprotective.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
165.39 KB

Oh search.

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

Dereine reports that we need this in order to solve a Views issue: #812970: Get Views Query substitutions working.

Applies cleanly, and the changes make sense.

Crell’s picture

Status: Reviewed & tested by the community » Needs work

We are absolutely not committing a 165KB patch of this magnitude with only 4 short comments. Marking to come back to when it's not the middle of DrupalCon.

chx’s picture

Status: Needs work » Needs review

There is nothing to work on, but let's discuss (although there is nothing to discuss).

dawehner’s picture

One usecase for this is the thing called views_substitions. It was a way to have something like constans in sql strings.

A developer can use :node-administer-nodes in the query and views will replace it later with the real value.
Therefore it have to access the strings of the parts of the conditions. Conditions are marked totally as protected so there is no way to see what's in.

Additional when you try to look which values are which in the query to fix the own code dpm($variable) does not show the informations you need. People could look at the api and see that there is a method to get the data but this patch makes it easier for the developers to look at the variables. Sure it should be clear that it's not recommended at all to change internal variables.

Berdir’s picture

You can use the getter methods to get access to the whole $conditions array, see http://drupal.org/node/310077. They return by reference, so you can change everything you want.

It is possible that there are some methods missing, but that's simply a bug and can be fixed without the need of a 164KB patch...

chx’s picture

We tried that we are mired in a battle over the connection getter as I linked in the top post. Are we going to do that over every. single. property?

Edit: you get the connection array, good, and then it has a number of protected properties...

Edit2: Do we really believe that we built an API that can cope with every usage ppl will come up in the next 5+years of Drupal 7 lifetime? We never released a restrictive API without an out (remember #DANGEROUS_SKIP_CHECK?)

Crell’s picture

As I discussed with dereine in CPH, the Views issue is that Views is trying to use :something as a placeholder format, which is already used by PDO. It's also trying to use hyphens in the placeholder name, which confuses PDO even more. Hilarity ensues. That's unrelated to the public/protected question.

chx’s picture

Title: protected is dangerous » We must document that protected is dangerous
Priority: Normal » Critical

I am changing this into a documentation issue. I have a deep set , maybe irrational fear that DBTNG does not allow the necessary amount of hackability because of its protected properties. However, for PHP 5.3.0 and on http://php.net/manual/en/reflectionproperty.setaccessible.php solves this problem. We should add a docblock saying "if you need to change foo but can't then you can always do the following":

class testClass {
  protected $y = 1;
}
$object = new testClass;
$reflectionObject = new ReflectionObject($object);
$property = $reflectionObject->getProperty('y');
$property->setAccessible(TRUE);
$property->setValue($object, 2);
chx’s picture

Status: Needs review » Needs work

of course, needs work.

chx’s picture

Status: Needs work » Needs review
FileSize
867 bytes
webchick’s picture

Priority: Critical » Normal

There is absolutely no reason a documentation issue intended for developers doing the very edgiest of edge cases is a release blocker, sorry. This can happen at any time.

chx’s picture

Priority: Normal » Critical

I totally and absolutely disagree. To release like this is very very dangerous. I bet two minutes after release we fill find holes. The original post already points to an issue being debated that is swept aside by this issue. Also, it's a comment, we can just comment, noone is hurt. :)

Last time I tried to comment DBTNG I was not allowed to. This is a way deeper concern that must be addressed in some way. I am throwing tantrums at poor Crell at random intervals for years now because of this. The dangers of this issue must not be underestimated. That I found a simple way that makes the above 160KB+ patch unnecessary is close to a miracle.

chx’s picture

Also note that I actually ran that code (by copypasting it into index.php) and it worked (verified by dumping $this->prefixes in the prefixTables method).

chx’s picture

Changed getter to "reference getter".

Also if it's not crystal clear I am scared sh!tless about releasing Drupal 7 and I am in damage control mode.

Mark Trapp’s picture

Shouldn't this also be documented for protected methods as well? Same principle, but ReflectionMethod::setAccessible() requires PHP >= 5.3.2, not merely 5.3.

Dries’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

I can't make sense of the documentation. Also, this is not going to block the release so changing the priority.

Mark Trapp’s picture

Thinking about this overnight, this seems like something that shouldn't be in the API documentation.

While it may be relatively new for PHP (and definitely new for Drupal core), reflection is a well-established concept in object-oriented programming, not something unique to the Drupal API that warrants a special mention in the code comments.

This seems like a thing that's better covered in a handbook page.

webchick’s picture

I guess the proper place to put this would actually be on api.drupal.org as a comment on... something.

But I think we need to get http://api.scratch.drupal.org/ running before that's possible.

chx’s picture

Priority: Normal » Critical

Dries, webchick and all. I do not think I have described the problem well.

The stake is Drupal's hackability.

Say you had a complex multisite where you wanted to peek into the tables of another site occassionally. In Drupal 6, you do that with changing a global variable. In Drupal 7, you simply can't.

I already linked #802514: changeConnection from hook_query_alter but there is #814312: Add getConnection() to SelectQueryExtender too.

Are these edge cases? Yes! But Drupal will be seen as broken for the people trying to do advanced stuff. That's what makes this issue critical, yes, release blocking: we are limiting Drupal 7 more than Drupal 6 was and we have no idea where those limitations lie.

We do have a simple solution at hand: document that we are still as flexible as necessary. It's a small patch. We can figure out wording and placement neither is a big deal.

But it'd be a grievous mistake to release Drupal 7 without communicating the problem and this way out.

chx’s picture

I have an even simpler fix. Let's quickly figure out a nice alias for http://drupal.org/node/310069 link to it from the code and then I will write a nice handbook page.

Berdir’s picture

handbook/database ? :) (EDIT: Oh, I assumed you wanted to link directly to a documentation page about the protected stuff, but that's the DBTNG documentation page. Then I'm totally for adding this link, the documentation there is great and deserves to be better available).

Also, I have some more oil for your fire: #922380: Custom PagerDefault count queries getter method is protected :)

Let's see if that issue proves my point in #8 or not ;) (Missing getter functions are a bug and should be fixed)

I still don't see why a documentation issue needs to be critical but let's get this in and everyone is happy again :)

chx’s picture

No need to link to the protected page itself, #922090: Add an alias to the Database API page is the webmasters issue about the alias.

Dries’s picture

I'm still not sure I understand the critical nature of this problem.

If you want to peek into the tables of another site, one should be able to create a new database connection not?

Crell’s picture

Priority: Critical » Normal

This is not critical. It's a documentation question at this point.

And yes, you can absolutely connect to another database with a new connection at any time to do what you want. That's completely irrelevant to this question.

Mark Trapp’s picture

@Dries,

The issue is that a module (or anything, really) that creates an object can't access that object's protected methods or properties, and in some (edge) cases, a module might need to.

The solution is to either change protected to public and warn people that the public methods are really for internal use only (the basis for the 164KB patch), or require a module that really needs to use protected methods and properties to use reflection to make protected properties and methods accessible at runtime (the basis for the documentation change).

chx’s picture

Priority: Normal » Critical

Crell, even Dries left this at critical. Don't be too proud of this technological terror you've constructed ;) ie. admit that there are use cases we have not thought of. It's funny that you guys try to press me to think of one -- if I could then we would have changed the API already, right? And the connection issue linked several times is suspiciously one.

mfer’s picture

Priority: Critical » Normal

Why is this critical?

Dries noted:

I'm still not sure I understand the critical nature of this problem.

If the information should be available outside the object then a patch to make the visibility public would be useful. Otherwise, documenting php oop visibility in Drupal does not seem useful. Its part of the language and the language has its own page for it. Are our code comments meant to teach the language and how to use it?

chx’s picture

Priority: Normal » Critical

@mfer, yes. Is this too hard to understand? This is the first time we add OOP to Drupal in a major way. It can be expected our developers are not familiar with OOP especially not with the superb wretched, broken, limited ways PHP implements it. And this particular reflection trick is only in 5.3.

chx’s picture

mfer’s picture

Priority: Critical » Major

I spoke with Crell and there are a few things:

  • We will set this to Major as a compromise for now.
  • The sweet trick in #11 should be documented (in a positive) way on how to alter protected properties as a general pattern. This should go in the handbook as that is where other documented entries like this have been placed.
  • Whether to make this public or protected seems to be a matter of tradeoffs. I discussed this with Crell and we looked at this from different directions. He said he is going to write some blog posts detailing his stance on public/protected, tradeoffs, and other software architecture stuff around this. These philosophical discussions happening here would be books (we all know how long Crell blog posts are). So, lets see his stance and we can use that as a discussion starting point.
  • Are there cases where having this as a protected method cannot be worked around though a hook, extending a class, or something else? If so, what are the specific details.

This may seem like a long way around to get this worked out. But, there are some different philosophies going on here and that's where some of this is happening. Lets see what these are and how we can move on from there.

webchick’s picture

Priority: Major » Normal

I'm setting this to normal for the last time. The next time I set it to anything else, it's going to won't fix.

webchick’s picture

Now, in terms of the actual bug, the big question in my mind deals with target audience and the proper place to put this documentation.

It seems to me the target audience of this change is someone who is attempting to write their own database backend for Drupal 7 (e.g. MongoDB, etc.) or perhaps someone trying to make their own equivalent of the Views module? Could we get clarification here? That would help us to evaluate whether the documentation that's in the patch is sufficient.

Then the second aspect of this is that if I were that person doing that edge case, where I would look for this type of tip? The current patch -- though you can't tell from the patch file -- would embed this documentation into the page at http://api.drupal.org/api/group/database/7. This seems mis-placed to me, at first glance. That page is for a general target audience trying to figure out how Drupal's database abstraction works. It covers topics like how to write basic queries, how placeholders work, how to make use of transactions, etc.

Intermingling text there that's intended for 0.00001% of the development target audience doesn't seem like a great idea to me. I again am curious why this can't be a comment at the bottom of that page. Done and done.

mfer’s picture

@webchick the issues is what is in #3. When chx ran into a roadblock on working this he turned to documentation. Do we make then public or not? That is the question.

chx’s picture

webchick, I can't give you clarification. I simply can't. That's the issue itself. I have no idea what the system is incapable of. If I knew, I would fix it.

It is possible that a particularly complicated query being passed back and forth in a sufficiently complex system can't do something it would want.

It's more likely that someone with many databases / prefixes will run into something.

This issue is about the protected variables / methods taking away flexibility.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
286 bytes
webchick’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Well, I definitely can't argue with that. :) A lot of people at that page on api.drupal.org have requested a cross-link to the database API docs in the handbook.

Committed #38 to HEAD.

Doesn't feel like we can set this to "fixed" yet though, because we're waiting on Crell for public/protected pros/cons. So... uh... "postponed (needs more info)" ?

skwashd’s picture

It seems that there is some confusion around private and protected.

As noted in http://aperiplus.sourceforge.net/visibility.php private is evil. On the other hand, protected is very useful. Protected allows something to be marked as internal, but accessible to classes which extend that object. Extending classes can implement getters and setters for the protected properties or wrapper methods for protected methods - this should of course include some kind of sanity checking.

The solution is to either change protected to public and warn people that the public methods are really for internal use only [...] --Mark Trapp

The whole purpose of protected is to flag methods/variables as internal and have that enforced. If you want everything public with documentation or comments indicating that something is internal or protected, you might as well stick with a PHP4 compatible OOP implementation.

Mark Trapp’s picture

@skwashd I wasn't necessarily advocating making everything public; I was pointing it out as the basis for the 165KB patch in #3.

I think the issue being raised is that Drupal historically has allowed daring third-party developers access "internal" (scare quotes) functions if they really, really needed to. That is, Drupal internal functions are allowed to be used if you know what you're doing, with the underscore in the name merely signifying "beware, there be dragons ahead."

In Drupal 7, this gentleman's agreement has ostensibly been broken as protecting methods and properties indicates that one really shouldn't be using those things unless one is extending the class, thereby reducing Drupal's "hackability." If, for example, an existing object has protected properties or methods, I, as a module writer, can't use them even if I want to.

So, the original idea was that the long-standing policy of allowing third-party developers to access internal functions easily should be kept, and the decision to use protected be reversed.

However, a developer in-the-know can and will use reflection to override the protected restriction to get what he or she needs, so while using protected might give off the wrong impression (that it's really not okay to mess with internal stuff), sufficient documentation should be enough to inform developers how to easily get around the visibility restrictions. An official handbook page would be enough to say "Yes, we're using protected, but if you know what you're doing, it's okay to override that; we won't hate you."

Pasqualle’s picture

another issue with protected value #702778: Multiple pagers on the same page

Damien Tournoud’s picture

I said a few times already that non-"public" modifiers are simply incompatible with our AOP-like programming style (hook_query_alter()).

The "pass by reference" getters are just a ugly, alien hack. All those should be replaced by public properties, but this is not a blocker for D7.

Anonymous’s picture

Status: Postponed (maintainer needs more info) » Closed (won't fix)

Given the amount of 'protected's I see in D8, I don't think this is an issue anymore.