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#3029] Avoid risking a TypeError when evaluating tokens for non-existent custom fields #22537

Merged

Conversation

jensschuppe
Copy link
Contributor

Overview

Fix for core/dev#3029

Before

Evaluating tokens for non-existent custom fields fails with a 500 TypeError.

After

No error.

Technical Details

Due to type hinting in CRM_Core_EntityTokens::getCustomFieldName() a TypeError was being issued when the custom field in question did not exist. Instead, an exception is now being thrown, which is being caught in the currently only calling method CRM_Core_EntityTokens::getCustomFieldValue() which returns NULL instead of a formatted custom field value.

Comments

Maybe simply returning NULL (with PHP 7.1 supporting nullable return types) is easier? @eileenmcnaughton

@civibot
Copy link

civibot bot commented Jan 17, 2022

(Standard links)

@civibot civibot bot added the master label Jan 17, 2022
@eileenmcnaughton
Copy link
Contributor

@jensschuppe I'm OK with this - but it could go against the rc branch (5.46) since it's a regression. It's obscure enough I'm happy just to merge to master if you prefer

In a perfect world we would have some sort of sytem check for templates with invalid custom field tokens and / or bubble up a message that logged in users would see so they know something unexpected is happening

…ror when evaluating tokens for non-existent custom fields
@jensschuppe jensschuppe changed the base branch from master to 5.46 January 18, 2022 09:04
@civibot civibot bot added 5.46 and removed master labels Jan 18, 2022
@jensschuppe
Copy link
Contributor Author

Changed base branch to 5.46 as suggested by @eileenmcnaughton.

In a perfect world we would have some sort of sytem check for templates with invalid custom field tokens and / or bubble up a message that logged in users would see so they know something unexpected is happening

Since we can't be sure that everything that looks like a CiviCRM token in a document actually is supposed to be a token, the replacement system should gracefully ignore everything it doesn't recognize. If the user were to be informed about errors regarding tokens, this would have to be a separate check, e.g. being run from within the template editing screen.

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

thanks @jensschuppe - I've given this Merge on pass

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001 seamuslee001 merged commit 97286b7 into civicrm:5.46 Jan 20, 2022
@jensschuppe jensschuppe deleted the fix/typeErrorCustomFieldTokens branch January 20, 2022 07:32
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.

3 participants