See #1513970: Convert SearchQuery to PSR-0 and #1513210: Meta: Start converting module provided classes to PSR-0 for more information.

This isn't strictly necessary because these two files are placed in files which are currently always includes. However, I guess we would all prefer if we could make these optional I guess :)

What I'm wondering is if we can find a way to prevent us from writing extend('Drupal\Core\Pager\PagerDefault') a million times. Could be a default namespace for extenders, or some sort of dependency injection? Would add a dependency on drupal_container() on the database component, as that works now.

Follow up: #1541892: Convert TableSort to PSR-0

CommentFileSizeAuthor
#7 1541684-pager-7.patch35.88 KBaspilicious
#5 1541684.patch33.87 KBRobLoach
#2 PagerSelectExtender.patch33.87 KBRobLoach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Title: Convert PagerDefault and TableSort db extenders to PSR-0 » Convert PagerDefault db extender to PSR-0

Let's split TableSort off to its own issue to keep the patches simple :-) #1541892: Convert TableSort to PSR-0.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Active » Needs review
FileSize
PagerSelectExtender.patch33.87 KB

Status: Needs review » Needs work

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

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Looks like most if not all of the fails are caused by this:

Argument 2 passed to Drupal\Core\Database\Query\PagerSelectExtender::__construct() must be an instance of Drupal\Core\Database\Query\Connection, instance of Drupal\Core\Database\Driver\mysql\Connection given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Select.php on line 299 and defined

Sounds like a missing use statement.



+++ b/core/modules/node/node.admin.incundefined
@@ -447,7 +447,9 @@ function node_admin_nodes() {
-  $query = db_select('node', 'n')->extend('PagerDefault')->extend('TableSort');
+  $query = db_select('node', 'n')
+    ->extend('Drupal\Core\Database\Query\PagerSelectExtender')

This was easy to type/remember before the conversion, now it's a lot harder. Also not sure if I like the name.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs work » Needs review
FileSize
1541684.patch33.87 KB

The PagerSelectExtender is a SelectExtender, which helps with pagers. Not sure what a better name for it would be.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.phpundefined
@@ -0,0 +1,172 @@
+   * @param SelectQueryInterface $query

Full namespace paths needed in documentation.

Yeah the extenders will be harder to remember after the psr-0 conversion but we can't do anything about that if we don't want to introduce some kind of black magic. And the name sounds "sane". PagerDefault isn't a good name either for what it does.

18 days to next Drupal core point release.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
1541684-pager-7.patch35.88 KB

Rerolled, lets get this in!

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented

#7: 1541684-pager-7.patch queued for re-testing.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community
RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Issue tags: +Needs change record, +PSR-0

I'm tagging change notification early so we don't forget.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Convert PagerDefault db extender to PSR-0 » Change notification for: Convert PagerDefault db extender to PSR-0
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Thanks! Committed/pushed to 8.x.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

We already have a change notice for the Search query extender, should we re-use that or create a new one? TableSort will follow soon....

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Just one for all extenders seems alright to me.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Active » Needs review

Review, fix the todo and make it better. Thnx!

http://drupal.org/node/1621062

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Title: Change notification for: Convert PagerDefault db extender to PSR-0 » Convert PagerDefault db extender to PSR-0
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks good to me!

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Priority: Critical » Normal
meba’s picture

meba CreditAttribution: meba commented

How does this affect Developer Experience?

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

See the change notice listed on top of this page: http://drupal.org/node/1621062

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

afsdfsdafd