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#2743 fix api v3 to not unnecessarily load options #21099

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

dev/core#2743 fix api v3 to not unnecessarily load options

Before

Campaign options loaded for verification even when its already an int

After

Only loaded if needed

Technical Details

We don't need to validate if already numeric

Comments

@colemanw this is actually very similar to @pfigel's hack & I think it makes sense for the rc

@civibot
Copy link

civibot bot commented Aug 11, 2021

(Standard links)

@civibot civibot bot added the 5.41 label Aug 11, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the camp54 branch 2 times, most recently from 9686d46 to 7d6056f Compare August 11, 2021 22:32
(!empty($fieldInfo['pseudoconstant']) || !empty($fieldInfo['options']) || $fieldName === 'campaign_id')
// if it is already numeric AND it is an FK field we don't need to validate because
// sql will do that for us on insert (this also saves a big lookup)
&& (!is_numeric($fieldValue) || empty($fieldInfo['FKClassName']))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have re-written this line 8 times so I'm gonna write out what I'm doing

We will validate if it's a a pseudoconstant, has options or is campaign id

AND it is not an FK OR the value is not numeric

Ie if it is an FK & we have a string then validate
if it's numeric & it's controlled by option values then validate

but don't validate a numeric FK field

@colemanw
Copy link
Member

This looks good code-wise and makes a lot of sense. It's also heavily covered by tests.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Aug 11, 2021
@eileenmcnaughton
Copy link
Contributor Author

@colemanw this caused 3 test failures - in all 3 cases they were upset about the subtle difference in the error message. I kept one test but removed the error message check & I actually removed the other 2. In general I think we have an overkill of these tests that check failure messages so keeping 1 was enough IMHO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.41 merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants