Problem/Motivation

We now have the method Drupal\Core\Database\Connection::prefixTable($table) which returns the prefix for the table. In D9 and before there was the possibility to have different prefixes for different tables. In D10 there is only a single prefix for all tables. The method and its parameter suggest that it is still possible to have per table prefixing. This does not improve the developer experience.

Proposed resolution

Deprecate method Drupal\Core\Database\Connection::prefixTable($table) and create the new method method Drupal\Core\Database\Connection::getPrefix(). The new method has no parameters. It just return the class variable $prefix.

Remaining tasks

TBD

User interface changes

None

API changes

See propesed solution.

Data model changes

None

Release notes snippet

TBD

Issue fork drupal-3257201

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

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie commented

This issue is postponed on #3124382: Remove per-table prefixing.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Create the new method Drupal\Core\Database\Connection::setPrefix() and deprecate Drupal\Core\Database\Connection::prefixTable($table) » Create the new method Drupal\Core\Database\Connection::getPrefix() and deprecate Drupal\Core\Database\Connection::prefixTable($table)
elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Assigned: Unassigned » elber
elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Assigned: elber » Unassigned
beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Assigned: Unassigned » beatrizrodrigues

I'll work on that.

beatrizrodrigues opened merge request !1727

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Assigned: beatrizrodrigues » Unassigned
Status: Active » Needs review

I applied the proposed solution and created the change record too. I hope everything is correct.

daffie’s picture

daffie commented
Status: Needs review » Needs work

Back to needs work for the unresolved threads.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Postponed

Need to wait for #3124382: Remove per-table prefixing

daffie’s picture

daffie commented
Status: Postponed » Needs work

#3124382: Remove per-table prefixing has landed.

ravi.shankar made their first commit to this issue’s fork.

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

murilohp’s picture

murilohp commented
Assigned: Unassigned » murilohp
murilohp’s picture

murilohp commented
Assigned: murilohp » Unassigned
Status: Needs work » Needs review

Hey everyone! I had to rebase the branch with D10 branch, I also fixed some stuff, thank you for the commits @beatrizrodrigues and @ravi.shankar, you've helped a lot!

@daffie, it would be nice to have your thoughts here again.

Thanks!

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

The MR looks good to me.
The old method is deprecated with a deprecation message test.
The IS and the CR are in order.
For me it is RTBC.

catch’s picture

catch
he/him
Primary language English
commented

In general we're only adding deprecations in Drupal 10 for removal in Drupal 11 if it's necessary to fix Drupal 10-blocking issues. Most general clean-up type changes would get deferred to 10.1.x at this point. This is because phpstan will warn people about all deprecations regardless of versions, but if replacements aren't in 9.x, then they can't be resolved without breaking Drupal 9 compatibility.

Could we not add the method to 9.4.x with a deprecation for Drupal 10? Even if the method only works for single table prefixes in 9.4 this might be fine - but I did not properly review the issue yet, so apologies if this has been asked and answered already.

catch’s picture

catch
he/him
Primary language English
commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Needs work

Moving to 10.1.x since this has missed 9.4.x, MR needs an update to change the deprecation message.

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review

Made changes as per comment #18, please review.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All usage of the old method has been replaced by the new method.
A deprecation message and test has been added to the MR.
I have updated the CR.
For me it is RTBC.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Couple of comments on the MR - not sure they warrant un-RTBCing

  • catch committed 20e4bae on 10.1.x 
    Issue #3257201 by murilohp, ravi.shankar, beatrizrodrigues, daffie,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 20e4bae and pushed to 10.1.x. Thanks!

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3281962: Add @internal to Database API methods that should only be called by driver code, +#3118629: Add @internal to public database API methods where appropriate

Setting @internal has not to do with the visibility, the method itself must remain public. The usage of the method in migrate is borderline - it seems to me is more to circumvent a shortcoming in the DB API (unable to process identifiers longer than 63 chars in PgSQL) than anything else.

We are discussing to make DB API methods @internal for usage within db drivers here #3118629: Add @internal to public database API methods where appropriate and here #3281962: Add @internal to Database API methods that should only be called by driver code, will add this one there.

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

Updated and published the change record.