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

Fixed: Edge-case in title_format for CT's with no textual fields #7108

Merged
merged 2 commits into from
Oct 22, 2017

Conversation

bobdenotter
Copy link
Member

giphy-downsized-large

$keys = array_keys($fields);

return reset($keys);
return array_keys($contentType['fields']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is returning all fields then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method itself returns an array of fieldnames that are considered to cobble together the title. If no suitable column is found at all, it'll either display (no title) or something put together from fields. For example:

dummy:
    name: Dummies
    singular_name: Dummy
    fields:
        turtlesallthewaydown:
            type: select
            values: [ A-tuin, Donatello, Rafael, Leonardo, Michelangelo, Koopa, Squirtle ]
        image:
            type: image
        datum:
            type: date

No title field, no title_format and no type: text fields. This CT would show "Donatello" or "Koopa" or the image filename in the listing overview, and "latest changes". It's not ideal, but IMHO better than before.

Before:

screen shot 2017-10-20 at 14 10 29

After:

screen shot 2017-10-20 at 14 12 31

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't begin to tell you how wrong I find this approach 😢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the change introduced in #7067 tries to return a non-existing variable … This needs tests!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the change introduced in #7067 tries to return a non-existing variable

Yes, that is exactly what this PR is fixing: that piece of code that came from elsewehere wasn't adapted properly.

This needs tests!

When we discussed this, I thought we decided that we'd just fix it up to fix the bugs we're seeing, since it is marked "deprecated" and will get replaced sooner or later regardless.

@bobdenotter
Copy link
Member Author

Should be GTG now.

Copy link
Contributor

@GwendolenLynch GwendolenLynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ tests ❤️

@GwendolenLynch GwendolenLynch merged commit 0053bda into 3.4 Oct 22, 2017
@GwendolenLynch GwendolenLynch deleted the hotfix/title-format-edge-case branch October 22, 2017 09:41
@GwendolenLynch GwendolenLynch added this to the Bolt 3.4 - Feature release milestone Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants