Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include fields’ UIDs in their content column names #6922

Closed
brandonkelly opened this issue Sep 29, 2020 · 16 comments
Closed

Include fields’ UIDs in their content column names #6922

brandonkelly opened this issue Sep 29, 2020 · 16 comments
Labels
enhancement improvements to existing features system architecture 🏗️

Comments

@brandonkelly
Copy link
Member

There are a couple cases where custom fields’ content table columns could get dropped or renamed unexpectedly. For example, if two global fields with the same handle are added via the project config (#6536), only one column will be created for both, and when one of those fields are deleted, the column will be dropped with it, resulting in potential data loss and SQL errors due to the remaining field still expecting its column to be present.

We can fix this in Craft 4 by including the first ~8 characters of fields’ UIDs in their content column names. For example instead of field_body, the column could be named field_body_08f8ec90. That way Craft can be 100% sure it’s deleting/renaming the correct column.

@rob-baker-ar
Copy link

Would this potentially lead to being able to handle a fewer number of columns (fields) on the content table?

i.e. This: https://dev.mysql.com/doc/refman/8.0/en/column-count-limit.html

MySQL has hard limit of 4096 columns per table, but the effective maximum may be less for a given table. The exact column limit depends on several factors:

The maximum row size for a table constrains the number (and possibly size) of columns because the total length of all columns cannot exceed this size. See Row Size Limits.

The storage requirements of individual columns constrain the number of columns that fit within a given maximum row size. ...

Storage engines may impose additional restrictions that limit table column count. For example, InnoDB has a limit of 1017 columns per table...

I'm no expert in max columns for MySQL but I did have the unfortunate experience years ago of having to try and resurrect an old (v2) Expression Engine multi-site (it used ~100 fields for each site and there was more than 10 sites) set-up that fell foul of these limits.

@brandonkelly
Copy link
Member Author

No this is unrelated, however we are going to drastically reduce the likelihood of running into column limits in Craft 4 (see #1524).

@brandonkelly
Copy link
Member Author

This has been implemented for 3.7. Went with a new random string instead of the first 8 characters of the UID because we are only doing this for newly-saved fields (for now), and needed some way to differentiate between fields that have a random string in their column vs fields that haven’t been saved post-3.7 yet, so that differentiation may as well be the existence of a new columnSuffix value on the field.

@brandonkelly brandonkelly modified the milestones: 4.0, 3.7 Mar 30, 2021
@brandonkelly brandonkelly mentioned this issue Mar 30, 2021
@mmikkel
Copy link
Contributor

mmikkel commented Mar 30, 2021

@brandonkelly Won't this change break any custom queries (including "advanced" element queries using craft\db\Query methods like where()) that reference existing field column names (e.g. content.field_{fieldHandle}), once fields are re-saved on 3.7 and their column names are changed to include that random suffix? Or is the suffix only applied to new fields?

@steverowling
Copy link
Contributor

@brandonkelly Won't this change break any custom queries (including "advanced" element queries using craft\db\Query methods like where()) that reference existing field column names (e.g. content.field_{fieldHandle}), once fields are re-saved on 3.7 and their column names are changed to include that random suffix? Or is the suffix only applied to new fields?

@mmikkel, @brandonkelly did say that this would only apply to new fields for now:

… because we are only doing this for newly-saved fields (for now)…

I do share your concerns, though, that this will break the ability to use select() or where() clauses in advanced element queries, which are extremely useful in making lightweight queries for limited datasets, instead of returning complete Entry models.

@mmikkel
Copy link
Contributor

mmikkel commented Mar 31, 2021

@brandonkelly did say that this would only apply to new fields for now

I hope that's the case, but "newly-saved" sounds a bit ambiguous to me – could mean new fields, but could also mean "any field saved after the update".

I do share your concerns, though, that this will break the ability to use select() or where() clauses in advanced element queries

I don't think it would break that ability – although it will certainly be more cumbersome, as field column names will essentially be random. From looking briefly at the source, I think the new ElementHelper::fieldColumn() or ElementHelper::fieldColumnFromField() methods could help with this, but you'd either have to fetch the actual field or manually look up the suffix, if I'm reading things right.

Mostly, I'm concerned about existing fields potentially having their column name changed, so looking forward to some clarification on that.

@brandonkelly
Copy link
Member Author

Yes it will affect existing fields as well. I chose to only make the change on field save (as opposed to all at once via a migration) so that the change happens on your own terms. We’ll be explaining the change in the 3.7 upgrade guide and noting that if you are referencing field columns in custom queries / conditions, you should go through and re-save those fields up front and update the column references.

@mmikkel
Copy link
Contributor

mmikkel commented Mar 31, 2021

@brandonkelly Thanks for clarifying.

Personally I feel like this is a breaking change that should’ve waited for Craft 4.

I also think that not doing it as a migration probably makes the change more difficult to deal with, as the column names changing is essentially made a side effect to the simple act of saving a field (a thing that can occur, for a multitude of reasons, months down the line from the actual 3.7 upgrade).

@brandonkelly
Copy link
Member Author

I also think that not doing it as a migration probably makes the change more difficult to deal with, as the column names changing is essentially made a side effect to the simple act of saving a field (a thing that can occur, for a multitude of reasons, months down the line from the actual 3.7 upgrade).

True but renaming fields on save is nothing new. Each time you save a field Craft already renames it and adjusts its column type based on its new handle and settings. And for Matrix sub-fields, the column name will change if the parent block type handle changes as well (not to mention the entire content table name could change if the Matrix field’s handle changes).

That said, I’m not opposed to limiting the change to new fields, plus fields whose getContentColumnType() method is now returning an array. That should still be enough to solve the naming conflict issue.

@brandonkelly
Copy link
Member Author

…and done: 5ac110d

@mmikkel
Copy link
Contributor

mmikkel commented Mar 31, 2021

Cool, I think that's probably a good call – nice to not have to worry about existing code breaking w/ the 3.7 upgrade, and the multi-column feature is going to be great to have, going forward.

If and when the time comes that all fields need suffixes (Craft 4.0?), I think a migration would be fine at that point. There's a big difference between a column or table name changing in a predictable way when a handle is changed, vs. Craft non-expectedly changing a column name in a non-predictable way, regardless of why the field was saved (for example, just to change an instruction or setting) – so I think the original approach here would've probably caused trouble.

@MoritzLost
Copy link
Contributor

MoritzLost commented Aug 26, 2021

@brandonkelly Is there any documentation on the suggested / preferred way of writing custom select() or where() statements given the new field suffixes? The closest I've come is this, which is pretty verbose for a simple select:

{% set body_field = craft.app.fields.getFieldByHandle('body') %}
{% set ElementHelper = create('craft\\helpers\\ElementHelper') %}
{% set body_column = ElementHelper.fieldColumnFromField(body_field) %}

{% set entries = craft.entries().select(['title', body_column]).all() %}

Is there a better way to get the column suffix and/or accessing the static ElementHelper methods without creating an object (besides adding the required functions in a Twig extension)?

By the way, I noticed that the new methods (ElementHelper::fieldColumn and ElementHelper::fieldColumnFromField don't appear in the API documentation, why is that?

@brandonkelly
Copy link
Member Author

@MoritzLost I wouldn’t instantiate an ElementHelper instance in your template; the class should really be marked as absract as it’s not meant to be instantiated.

The easiest way to do it would be to just hardcode the column name in your template. It won’t change between environments or anything.

By the way, I noticed that the new methods (ElementHelper::fieldColumn and ElementHelper::fieldColumnFromField don't appear in the API documentation, why is that?

The class reference is a bit behind, as the library it’s based on isn’t compatible with PHP 7.1+ syntax yet. (Fixing that is a WIP.)

@peckanthony
Copy link

The easiest way to do it would be to just hardcode the column name in your template. It won’t change between environments or anything.

I'm working on a plugin that needs to be shared across many different sites and need to do advanced queries on Entries regularly. Hardcoding al the needed columns will bring extra overhead, is there a way to determine a fields db column name in PHP?

@MoritzLost
Copy link
Contributor

@peckanthony You can use the helper method ElementHelper::fieldColumnFromField!

@danbrellis
Copy link

danbrellis commented Aug 17, 2022

The easiest way to do it would be to just hardcode the column name in your template. It won’t change between environments or anything.

Just FYI for anyone coming here looking to select a custom field in a twig template (and not wanting to hardcode the field name), the field object now includes a columnSuffix property to reference:

{% set body_field = craft.app.fields.getFieldByHandle('body') %}
{% set body_column = "field_body_" ~ body_field.columnSuffix %}
{% set entries = craft.entries().select(['title', body_column ~ ' as body']).all() %}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements to existing features system architecture 🏗️
Projects
None yet
Development

No branches or pull requests

7 participants