Allow source counts to be cached: implement ::doCount() instead of ::count() [#3190804]

Problem/Motivation

See \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::count() + \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::$cacheCounts.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#12	metatag-n3190804-12.patch	1.03 KB	DamienMcKenna
#12	8.x-1.x: PHP 7.3 & MySQL 5.7, D8.9.13 391 pass PHP 7.4 & MySQL 5.7, D9.2 395 pass PHP 8.0 & MySQL 5.7, D9.2 389 pass, 2 fail
#10	metatag-n3190804-10.patch	1.02 KB	DamienMcKenna
#10	8.x-1.x: PHP 7.3 & MySQL 5.7, D8.9.13 391 pass PHP 8.0 & MySQL 5.7, D9.2 389 pass, 2 fail PHP 7.4 & MySQL 5.7, D9.1 391 pass
#2	3190804-2.patch	1.01 KB	Wim Leers
#2	8.x-1.x: PHP 7.3 & MySQL 5.7, D8.9.11 381 pass, 2 fail PHP 7.3 & MySQL 5.7, D9.1 381 pass, 2 fail PHP 7.4 & MySQL 5.7, D9.1 391 pass PHP 7.2 & MySQL 5.5, D8.9 389 pass, 2 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

4 January 2021 at 10:57

Wim Leers created an issue. See original summary.

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 4 January 2021 at 10:58

Status:

Active

» Needs review

File	Size
3190804-2.patch	1.01 KB
8.x-1.x: PHP 7.3 & MySQL 5.7, D8.9.11 381 pass, 2 fail PHP 7.3 & MySQL 5.7, D9.1 381 pass, 2 fail PHP 7.4 & MySQL 5.7, D9.1 391 pass PHP 7.2 & MySQL 5.5, D8.9 389 pass, 2 fail

Comment #3

4 January 2021 at 11:08

Status:

Needs review

» Needs work

The last submitted patch, 2: 3190804-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 4 January 2021 at 11:33

I cannot reproduce those failures. Re-testing against 9.1 in case that makes a difference.

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 4 January 2021 at 11:59

Ah, right, this requires #3190815: Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O) 😬

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 4 January 2021 at 11:59

Title:	Allow source counts to be cached: implement ::doCount() instead of ::count()	» [PP-1] Allow source counts to be cached: implement ::doCount() instead of ::count()
Status:	Needs work	» Postponed

Comment #7

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 4 January 2021 at 12:13

Thanks for the patch and improvement, I look forward to the core change being committed so we can take advantage of this.

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 4 January 2021 at 17:32

Issue tags:

+Novice

@DamienMcKenna: wow that was fast! 😄

+++ b/src/Plugin/migrate/source/d7/MetatagFieldInstance.php
@@ -137,7 +137,7 @@ class MetatagFieldInstance extends DrupalSqlBase {
-  public function count($refresh = FALSE) {
+  public function doCount() {

Nit: should be protected.

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 17 May 2021 at 08:56

Title:	[PP-1] Allow source counts to be cached: implement ::doCount() instead of ::count()	» Allow source counts to be cached: implement ::doCount() instead of ::count()
Status:	Postponed	» Needs review

#3190815: Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O) landed! 🥳

Comment #10

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 17 May 2021 at 11:36

File	Size
metatag-n3190804-10.patch	1.02 KB
8.x-1.x: PHP 7.3 & MySQL 5.7, D8.9.13 391 pass PHP 8.0 & MySQL 5.7, D9.2 389 pass, 2 fail PHP 7.4 & MySQL 5.7, D9.1 391 pass

This would break compatibility with 8.9, so let's see if we can have both functions.

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 17 May 2021 at 13:53

+++ b/src/Plugin/migrate/source/d7/MetatagFieldInstance.php
@@ -141,4 +141,11 @@ public function count($refresh = FALSE) {
+  public function doCount() {

Should still be changed to protected 😅

Comment #12

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 17 May 2021 at 14:01

File	Size
metatag-n3190804-12.patch	1.03 KB
8.x-1.x: PHP 7.3 & MySQL 5.7, D8.9.13 391 pass PHP 7.4 & MySQL 5.7, D9.2 395 pass PHP 8.0 & MySQL 5.7, D9.2 389 pass, 2 fail

2 files were hidden/shown/deleted

File	Size
3190804-2.patch	1.01 KB
8.x-1.x: PHP 7.3 & MySQL 5.7, D8.9.11 381 pass, 2 fail PHP 7.3 & MySQL 5.7, D9.1 381 pass, 2 fail PHP 7.4 & MySQL 5.7, D9.1 391 pass PHP 7.2 & MySQL 5.5, D8.9 389 pass, 2 fail
metatag-n3190804-10.patch	1.02 KB
8.x-1.x: PHP 7.3 & MySQL 5.7, D8.9.13 391 pass PHP 8.0 & MySQL 5.7, D9.2 389 pass, 2 fail PHP 7.4 & MySQL 5.7, D9.1 391 pass

Doh.

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 14 July 2021 at 12:46

Status:	Needs review	» Needs work
Issue tags:	-Novice

FYI: the two failures in #12 on PHP 8 + Drupal 9.2 are the same two that exist in HEAD: https://www.drupal.org/pift-ci-job/2057145.

Also, the change in #10 + #12 compared to #12 breaks the performance improvement that #3190815: Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O) made possible.

If you want to support both simultaneously, you'll need to make count() smarter: you'll need to update its logic to be conditional depending on the Drupal core version…

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 29 September 2021 at 13:07

With #3190818: Allow source counts to be cached: implement ::doCount() instead of ::count() also committed … and with time having passed … I'm wondering if you'd be willing to commit this without BC layer for Drupal 8.9, @DamienMcKenna?

Comment #15

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 22 November 2021 at 22:22

Parent issue:

» #3203686: Plan for Metatag 8.x-1.17

With Drupal 8 no longer supported, I think this can go in the next release.

Comment #16

9 December 2021 at 19:28

DamienMcKenna committed 0133080 on 8.x-1.x authored by Wim Leers

Issue #3190804 by DamienMcKenna, Wim Leers: Allow source counts to be...

Comment #17

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 9 December 2021 at 19:28

Status:

Needs work

» Fixed

Committed. Thank you.

Comment #18

23 December 2021 at 19:29

Status:

Fixed

» Closed (fixed)

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

Allow source counts to be cached: implement ::doCount() instead of ::count()

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Parent issue

Related issues

Referenced by

Thank you to these Drupal contributors

News items

Our community

Documentation

Drupal code base

Governance of community