Problem/Motivation

A new property was added in #839444: Make serializer customizable for Cache\DatabaseBackend but doesn't use constructor property promotion.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3419356

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acbramley created an issue. See original summary.

Keshav Patel made their first commit to this issue’s fork.

Keshav Patel opened merge request !6467

taraskorpach made their first commit to this issue’s fork.

taraskorpach opened merge request !6472

taraskorpach’s picture

taraskorpach CreditAttribution: taraskorpach as a volunteer and at Drupal Ukraine Community commented
Status: Active » Needs review

I've created another merge request to avoid overwriting anyone's changes. @acbramley, could you take a look? Is it finally a "property promotion"?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#3419352: DatabaseBackend doesn't handle string typed $max_rows with $serializer

MR 6472 looks is what required but will have commit collision with #3419352: DatabaseBackend doesn't handle string typed $max_rows with $serializer

taraskorpach’s picture

taraskorpach CreditAttribution: taraskorpach as a volunteer and at Drupal Ukraine Community commented

I've noticed this issue as well. Since I'm relatively new, could you explain how such conflicts are handled within the Drupal issue workflow? Will the conflicts be resolved in the issue that is merged first, or is there another way?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

As the second one is RTBC already, let's keep this issue for rebase/re-roll

taraskorpach’s picture

taraskorpach CreditAttribution: taraskorpach as a volunteer and at Drupal Ukraine Community commented

Sounds great, thanks

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Postponed

Postponing on #3419352: DatabaseBackend doesn't handle string typed $max_rows with $serializer soon as that lands this just needs to be rebased and put back in review

acbramley changed the visibility of the branch 3419356-use-constructor-property to hidden.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented
Status: Postponed » Needs work

Blocker is in :)

taraskorpach’s picture

taraskorpach CreditAttribution: taraskorpach as a volunteer and at Drupal Ukraine Community commented
Status: Needs work » Needs review

Looks mergeable now.

Somewhat off-topic: PHPCS suggests that the code $this->maxRows = $max_rows === NULL ? static::DEFAULT_MAX_ROWS : $max_rows; could be simplified to $this->maxRows = $max_rows ?? static::DEFAULT_MAX_ROWS;. Is it worth creating an issue for this?

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented
Status: Needs review » Reviewed & tested by the community

Looking good, thanks!

Re #14 - yes that can definitely be tidied up in another issue :)

  • longwave committed 5ce8edea on 11.x 
    Issue #3419356 by taraskorpach, acbramley: Use constructor property...
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Fixed

Committed 5ce8ede and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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