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

WorkflowMessage - Enable strict parsing of annotations #25818

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

totten
Copy link
Member

@totten totten commented Mar 15, 2023

Overview

Not sure it's a good or bad idea. Spot-checked some local tests, and they seemed OK. Let's see what Jenkins thinks.

Not sure it's a good idea. Let's see what Jenkins thinks.
@civibot
Copy link

civibot bot commented Mar 15, 2023

(Standard links)

@civibot civibot bot added the master label Mar 15, 2023
@totten totten changed the title WorkflowMessage - Enable strict parsing of attributes WorkflowMessage - Enable strict parsing of annotations Mar 15, 2023
@eileenmcnaughton
Copy link
Contributor

@totten jenkins is positively cheerful - but does it matter that @braders patch isn't merged yet in terms of this one?

@totten
Copy link
Member Author

totten commented Mar 15, 2023

@eileenmcnaughton The two patches might interact. (Not at a git level -- but functionally.) Since I think the other is probably more important (this one was on a lark), I'd suggest merging #25790 first and then re-run the test here.

@eileenmcnaughton
Copy link
Contributor

@totten ok - let's merge the other - @colemanw has been ignoring it for 3 days - I doubt 2 more will matter + it would fail tests if it was a problem IMHO.

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

@totten it still passes

@totten
Copy link
Member Author

totten commented Mar 15, 2023

@eileenmcnaughton Wow, ok. I guess we can do this then.

  • Flipping this to "strict" is better from a core dev POV -- i.e. more likely to prevent bugs.
  • But, generally, "strictness" toggles should be available to downstream -- they represent a pressure-relief-valve for someone mixing versions of core/extensions in random ways.
  • It's kind of hard to care about as a hypothetical.

IMHO, the stricter policy is better than the looser policy -- but if anyone ever gets bitten by it, then it's discussion-welcome/patch-welcome to improve "strictness" toggles generally.

@eileenmcnaughton
Copy link
Contributor

OK - lets' merge this - I think stricter is better & we are better not to tie ourself in knots over what might be handy for extension writers - esp on functionality that probably isn't much in use in the wild

@eileenmcnaughton eileenmcnaughton merged commit b04de04 into civicrm:master Mar 15, 2023
@totten totten deleted the master-wf-strict branch March 15, 2023 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants