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

dev/core#2093 - Fix red error box on new individual form and ltrim typos and doubling-up of class attribute #18678

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Oct 6, 2020

Overview

**** I think this needs to be in 5.31. If not merged by then I'll rebase to be against it. ****

https://lab.civicrm.org/dev/core/-/issues/2093

  1. On dmaster.demo go to New Individual.
  2. Red error box.

Technical Details

In a recent commit this line was added:

$fieldAttributes['class'] .= ltrim($fieldAttributes['class'] ?? '' . ' crm-select2');

It has a couple issues:

  1. You can't use .= if the field isn't set yet, hence the red box.
  2. The .= is a typo to begin with. If the field is already set it doubles-up the class.
  3. Using ?? and . on the same line together requires brackets since the . always wins which leads to sneaky bugs. It turns out here it doesn't matter, but should have brackets. This isn't new to ?? - it used to come up sometimes with ? : too.

Comments

Has test. The existing version of a similar test didn't catch this because it didn't check custom fields.

@civibot
Copy link

civibot bot commented Oct 6, 2020

(Standard links)

@civibot civibot bot added the master label Oct 6, 2020
@seamuslee001
Copy link
Contributor

Looks good to me merging

@seamuslee001 seamuslee001 merged commit 81e4b16 into civicrm:master Oct 6, 2020
@demeritcowboy demeritcowboy deleted the class-typos branch October 6, 2020 20:50
@demeritcowboy
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants