Coming from #1858318: Make debugging EFQ easier by implementing hook_query_alter().

Nice DPQ module provides a more readable output for exposing queries. We might want to incorporate it into devel to expose the queries in this way, plus it already supports EFQ tag queries as for #2078425: Add support for EFQ and be even nicer coming from the issue above.

CommentFileSizeAuthor
#8 devel-8.x-1.x-nicedpq-2088593-8.patch16.66 KBdonquixote
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented

#2116311: Evaluate adding nicedpq() into devel crosspost in nice dpq module

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Nice DPQ's output looks nice indeed. It would sure be great to never again have to reformat SQL by hand!

However, there is no D8 version and the D7 version depends on X Autoload which is a hefty chunk of code. It does great things, but it's too big of a dependency to add to Devel.

I think we should look into providing a hook so that Nice DPQ can integrate with Devel, if the user chooses to install it, and provide basic functionality ourselves (including #1858318: Make debugging EFQ easier by implementing hook_query_alter().). Then we can put a recommendation and short explanation on our front page for those who require the "nice" functionality.

Alternatively, maybe donquixote would be willing to provide a light version that could be merged into Devel...

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Hi.
We had a similar discussion about KrumoNG.
One reason this is stuck is that for a devel-friendly version I would have liked more feedback.

The other reason is that I don't really want to code anything without either PSR-0 or PSR-4 or the PEAR-like PHP 5.2 compatibility mode of xautoload. I despise the Drupal 7 class registry with all my heart.

Unfortunately this leaves us with a problem: Either a leap to PHP 5.3, or introducing a scheme that is not really future-proof.

depends on X Autoload which is a hefty chunk of code.

True.
I think it would be in everyone's interest to slim this down.
X Autoload has some commitment to keep up with. I can't be removing features left and right. But I can reduce some overengineering.

The other option is to emulate a PSR-4 or PSR-0 or PEAR-like loader within devel, so no new dependencies are added to the devel module. I am doing this in other modules to avoid a dependency, and it works.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented

Thanks for getting back on this one @donquixote!, I'd postpone the krumoNG thing until we confirm if LadyBug gets in core or not.

I'd really want nicedpq() included in Devel so please any feedback you need, I'll try to help.

I agree with @salvis opinon of avoiding X Autoload dependency for Devel, I don't think it would be a good decision.

Could we start with D8? that would move the integration forward without touching the autoloading, right?

The other option is to emulate a PSR-4 or PSR-0 or PEAR-like loader within devel, so no new dependencies are added to the devel module. I am doing this in other modules to avoid a dependency, and it works.

Could you point to those changes in the modules to get a better idea if that would be the way to go?

donquixote’s picture

donquixote CreditAttribution: donquixote commented

http://drupalcode.org/project/crumbs.git/blob/33af04c8d508e42acbaf25de41...
lines 13 - 35.
This is the "PHP 5.2 compatibility mode", but similar stuff will be possible for PSR-0 or PSR-4.
I am using the underscore in _crumbs_autoload() to not unintendedly implement a yet unknown hook.

And yes we won't need this for D8, since this has PSR-0 / PSR-4 already included.
(btw, PSR-4 is still in progress, you are invited to participate #2083547: PSR-4: Putting it all together)

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Issue summary: View changes
Related issues: +#2083547: PSR-4: Putting it all together

Here's the piece of code that we're discussing for D7, I'd say this is what we'd need to add to have nicedpq without xautoload and greater rewrite.

/**
 * Crumbs autoloader.
 *
 * Takes the class name, strips the "crumbs_" prefix, converts underscores
 * to directory separators.
 *
 * For example, crumbs_InjectedAPI_describeMonoPlugin will be loaded
 * from lib/InjectedAPI/describeMonoPlugin.php.
 *
 * @param $class
 *   The name of the class to load.
 */
function _crumbs_autoload($class) {
  if (preg_match('#^crumbs_(.*)$#', $class, $m)) {
    $path = strtr($m[1], '_', '/');
    module_load_include('php', 'crumbs', "lib/$path");
  }
}

Shall we move this to d8 first?

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Yeah let's do this on D8 first.
I think dbtng is still mostly the same on D8 as it was on D7 (except in D8 it has namespaces).

I am going to produce a patch when I have time.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Issue summary: View changes
Status: Active » Needs review
FileSize
devel-8.x-1.x-nicedpq-2088593-8.patch16.66 KB

Let's play on github!
https://github.com/donquixote/drupal-devel/compare/8.x-1.x...8.x-1.x-nic...

EDIT:
I changed a lot of how nicedpq works.
There used to be an abstraction level with IndentedText argument instead of a plain string return value. This turned out to be totally unneeded.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Issue summary: View changes

(issue editing is confusing.)

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Identifiers like the Dpq namepace and the SelectPrinter and ConditionPrinter can be discussed.
The patch contains a lot of static, but I think this is acceptable for the given purpose. Especially this makes it easier to access the protected properties.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Btw: I prefer pull requests and inline github comments over reviews - esp if that is for trivial fixes.

EDIT:
I already see the line endings - duh.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented

Thanks a lot for the work on this!

I'd rather keep the issue discussion here instead of github, hope you don't mind.

  1. +++ b/devel.module
@@ -1467,9 +1468,31 @@ function dargs($always = TRUE) {
+      throw new \Drupal\devel\Dpq\DpqException('dpq() on entity queries cannot be used with the $return option.');

    Do we need a specific class for throwing dpq exceptions or shall we add a generic one / use the \Exception core one (?) 

  2. +++ b/devel.module
@@ -1467,9 +1468,31 @@ function dargs($always = TRUE) {
+    $query->addTag('devel_dpq');
...
+      $query->addMetaData('devel_dpq_name', $name);

    We could remove "devel" and just refer to dpq 

  3. +++ b/devel.module
@@ -1467,9 +1468,31 @@ function dargs($always = TRUE) {
+    $sql = \Drupal\devel\Dpq\SelectPrinter::printSelectQuery($query);

    use \Drupal\deve\Dpq and then just use Dpq?

Do we really need to add 3 different classes (conditionprinter, selectionprinter, util)? maybe having a generic for dealing with Dpq would be enough.

I wonder if d8 provides already some tools to mock database and pdo for us.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Do we need a specific class for throwing dpq exceptions or shall we add a generic one / use the \Exception core one (?)

I don't know. I think it is considered good practice to use dedicated exceptions. But I have not thought much about why. I think it is only relevant if someone tries to catch those exceptions.

We could remove "devel" and just refer to dpq

I think it is polite to avoid namespace pollution.

use \Drupal\deve\Dpq and then just use Dpq

So far the devel.module file does not have any use statements, so I kept it that way. But I am ok to change that.

Do we really need to add 3 different classes (conditionprinter, selectionprinter, util)? maybe having a generic for dealing with Dpq would be enough.

We need condition printer and select printer as separate classes with separate inheritance, to access the protected properties of condition and select. This may not be nice, but it is the only way.

Having a separate Util class for static methods that are innocent and standalone and do not depend on anything, seems like generally a useful pattern. These methods operate only with basic data types, they only depend on the arguments, they do not use any other classes, they have no side effects - they are as innocent as can be.

I wonder if d8 provides already some tools to mock database and pdo for us.

I found #2003342: Convert system module's database unit tests to phpunit where they say that "without more work around mocking a database connection", which I interpret as: Nope, core does not currently help us with that.

The good news is, we don't need a complete mock db connection. We only need the db connection to assist with building the query string, not to execute the query.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Assigned: Unassigned » salvis

Thanks for the feedback!

I'm adding @salvis to see if there's something he might want to add here. Otherwise I'm quite keen to add this to the D8 version and think move to how to implement it in D7

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

I've always been satisfied with unformatted SQL but if you guys think this is much better, then lets we can proceed. The code looks good. Lets give @salvis a few days to reply before committing.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Title: Evaluate adding nicedpq() into devel » Add nicedpq() into Devel
Assigned: salvis » Unassigned
Status: Needs review » Reviewed & tested by the community

Yes, this is very nice. Unformatted SQL is better than guessing, but non-trivial unformatted queries with a dozen pairs of parentheses are a pain to figure out without manually formatting first. Having dpq() do this is fantastic. And people who seek support in the queues have no excuse anymore to post unformatted queries. This encourages good habits.

What's more: it supports EFQ.

Thank you, donquixote, great job!

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Oh, there's one nit:

- * @param string $return
+ * @param bool|string $return
  *   Whether to return the string. Default is FALSE, meaning to print it
  *   and return $query instead.

$return is bool and nothing but bool. But having to specify FALSE in order to pass a $name is awkward. I'd be inclined to overload the second parameter so that dpq() can be called like dpm().

If you agree, we can keep the bool|string and follow through in a separate issue.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

I am a little confused by the original docblock comment for this parameter. It says type = string, but the description (and the rest of the code) says boolean. I take it this was a wrong docblock then.

The overload sounds useful. But as you say, this might be better in a new issue.
So,
- Option 1: Change the parameter docblock type hint to bool, do the rest in a follow-up.
- Option 2: Reduce to two parameters now.

I don't see a point in leaving bool|string. It just doesn't make sense.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Btw, Tim Plunkett on IRC (some time ago) made the point that SelectPrinter and ConditionPrinter do not actually print anything, but only return a string. Is there a better name for this?

donquixote’s picture

donquixote CreditAttribution: donquixote commented

And another: Should we use "use" statements for the classes within devel.module? And if not, should we write Drupal\ or \Drupal\?

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Yeah, the original docblock was clearly wrong. #8 is less wrong and I wouldn't hold it up for that if we agree that that's where we want to go anyway. But I won't object to Option 1... :)

Alternative names? SelectFormatter? SelectSerializer? SelectDumper?

Should we use "use" statements for the classes within devel.module? And if not, should we write Drupal\ or \Drupal\?

I don't have enough D8 experience to decide. moshe? pcambra?

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented

Regarding SelectPrinter and ConditionPrinter, as we have namespaces (Drupal\devel\Dpq) why just not go by Select & Condition?

I'd go for a followup for #17 and #18 to improve the parameter handling on dpq().

Regarding

Should we use "use" statements for the classes within devel.module? And if not, should we write Drupal\ or \Drupal\?

I don't see a pattern of using "use" (:D). Core goes for \Drupal::method() so we should do the same

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Reviewed & tested by the community » Needs work

Setting to needs work temporarily until we decide on the 2/3 minors left

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

why just not go by Select & Condition?

A bit bold, but it would work for me.

And

- Option 1: Change the parameter docblock type hint to bool, do the rest in a follow-up.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Seems like this stalled. Anyone willing to finish it up? I'm not clear whats needed.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Needs work » Closed (outdated)

Closing for lack of activity. Feel free to reopen if the issue still exists