The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
Follow-up from #2425127: Prevent PostgreSQL from creating duplicated index names within schema. The method name ensureIdentifiersLength does not document what it does accurately and should be renamed accordingly.
/**
* Make sure to limit identifiers according to PostgreSQL compiled in length.
*
* PostgreSQL allows in standard configuration no longer identifiers than 63
* chars for table/relation names, indexes, primary keys, and constraints. So
* we map all identifiers that are too long to drupal_base64hash_tag, where
* tag is one of:
* - idx for indexes
* - key for constraints
* - pkey for primary keys
*
* @param $identifiers
* The arguments to build the identifier string
* @return
* The index/constraint/pkey identifier
*/
protected function ensureIdentifiersLength($identifier) {
Proposed resolution
Rename the method to "hashIdentifierLength" and update the use of that method throughout core.
Remaining tasks
Need patchReview patchRun tests on PostgreSQL botWrite beta evaluation- Wait for #1060476: Multiple issues when PostgreSQL is used with non-public schema to be merged
- Reroll with method name change from #30
- Review
User interface changes
None.
API changes
Yes. However this is an internal protected method for pgsql driver.
Beta phase evaluation
| Issue category | Task because this is a clean up issue to make the method name more readable. |
|---|---|
| Issue priority | Normal because it improves the naming of methods/classes. |
| Prioritized changes | A follow-up issue from a Major PostgreSQL #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL. @xjm also has signed off on the change as reducing fragility (see #9). |
| Disruption | Disruptive for contributed modules subclassing postgres' schema class (extremely unlikely currently). |
| Comment | File | Size | Author |
|---|---|---|---|
| #34 | 2426579-34.patch | 7.36 KB | pk188 |
| #25 | 2426579-25.patch | 7.35 KB | bzrudi71 |
Comments
Comment #1
Nafes commentedAfter a bit of discussion with @Cottser, and @mparker17 at the DrupalNorth2015 code sprint, here is a patch to change the function's name to
hashIdentifierLength().@mparker17 remembered that
views-7.x-3.xhashes block machine names to ensure they don't exceed 32 characters, so we decided to look at the Views code to see whether it set a naming precedent. Turns out the code was done inviews_block_info(), not a function.We also checked to see if the hashing function was ported to Drupal 8, but we weren't able to find it.
Since we couldn't think of any more precedents, we brainstormed the best name for the function, and decided on
hashIdentifierLength().***
Would it also be worth updating the function docblock? It's okay, but the order of some words is a bit confusing.
Comment #2
Nafes commentedTagged as DrupalNorth. Have been completed on Drupal North code sprint.
Comment #3
daffie commentedThe new name seems to as a good solution.
All instances are renamed.
For me it is RTBC.
Comment #4
mradcliffeI reviewed the issue summary and corrected issue priority to more closely match what it should be after evaluating the issue via the Beta Evaluation process.
My one question about the patch is has it run through at least the Database test suite on PostgreSQL? I don't see any mention of running @Nafes' patch on drupalci test bot. I need to rebuild my local drupalci environment on this machine still, and will try to run that today.
Nice work on the patch, @Nafes. I really wanted to make it up to Canada too. :(
Comment #5
Nafes commentedThanks! Maybe next year in Montreal :)
Comment #6
Anonymous (not verified) commentedUpdating the issue tag to include a hashless DrupalNorth2015 on behalf of the Drupal North sprinting group.
Comment #7
mradcliffeI ran a postgresql 9.1.2 drupalci testbot run for Database and Entity groups, and all tests pass in those groups.
Comment #8
mradcliffeOops, did not mean to revert that tag removal.
Edit: or maybe it wasn't removed. It was an edit conflict and then I went ahead and saved anyway. Doesn't hurt to just use the hashless tag.
Comment #9
xjmThanks for the beta evaluation. I thought a bit about whether this change indeed makes sense during the beta despite being an API change. As the summary points out, this is a followup for a major earlier during the beta, and I agree that the current method name is indeed actively misleading, so it reduces fragility in that sense as well. The only disruption as far as I could tell would be for someone subclassing the PostgreQL driver, which seems extremely unlikely right now since D8 only recently works well on Postgres. :)
However, I'm not sure about the method name. It's not hashing the identifier length -- that would be, like, a hash of an integer.
How about something like
getIdentifierHash()?Comment #10
Nafes commentedLooks very good. After freezing method name I can re-create the patch.
Comment #11
mparker17+1 to
getIdentifierHash()! :)Comment #12
Nafes commentedThis is an updated patch for getIdentifierHash().
Comment #13
bzrudi71 commentedComment #16
daffie commentedComment #17
bzrudi71 commentedgetIdentifierHash()feels wrong. In 99% of all cases the identifier isn't hashed at all, just if it exceeds the 63 chars limit. So better something likehashOverlongIdentifiers()?Comment #18
nikky171091 commentedThis is an updated patch!
Comment #19
bzrudi71 commentedThanks @nikkR! We should test this :)
Comment #21
rajeevkLine indent corrected..
Comment #22
rajeevkComment #25
bzrudi71 commentedWell, #18 seems to fail for some reason and there is no interdiff, so let's start from scratch...
Comment #26
bzrudi71 commentedSadly the PG fail in #25 is unrelated and reminds me of #2565241: First test fails on postgres because of a stale connection (will report over there). And while waiting for the second bot run, what about
hashOverlongIdentifierNames():)Comment #27
googletorp commentedComment #29
daffie commentedFor me +1 for getIdentifierHash().
My review:
- I have checked that all uses of ensureIdentifiersLength() have been replaced.
- The testbot for PostgreSQL gives 3 fails and the daily testrun for PostgreSQL gives the same 3 fails. So they are not the result of this patch.
- I all looks good to me.
Comment #30
catchI'm not sure this is misleading, or that the new method name is better:
- this method ensures that the length of an identifier is within the limits set by postgres - at least that's the original reason for the naming. What exactly is misleading about it?
- it only hashes if the length is going to be too long, otherwise it keeps the original name, so getIdentifierHash() doesn't describe what it does.
What about something like normalizeIdentifierLength()?
Comment #33
andypost++, Needs reroll to change to
normalizeIdentifierLengthComment #34
pk188 commentedRe rolled.
Comment #35
jofitzPlease remember to remove the "Needs reroll" tag.
Comment #47
smustgrave commentedPostponing until #1060476: Multiple issues when PostgreSQL is used with non-public schema is merged in
Then we will need a reroll to address the renaming being requested by @catch in #30 and @andypost in #33
Updating issue summary
Please do not just reroll
Comment #49
xjmThe issue mentioned in #47 has been committed.
#30 still needs to be addressed, though. Plus, the previous patch is renaming the method without following the deprecation policy (because the proposed change and my previous signoff predate the deprecation policy). We should add a proper deprecation instead of the BC break.