Managed - Minor schema fixes

[colemanw]

Overview

Fix up a couple columns in the civicrm_managed schema.

Technical Details

• Increase size of module column to match civicrm_extension.full_name
• Makes name required and longer
• Makes cleanup required and declares explicitly the default value previously implied

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[totten]

• Increase size of module column to match civicrm_extension.full_name

👍

• Makes name required and longer

Longer names seem safe.

For flipping the required flag... I suppose the idea is that we've always understood it to be required. The due-diligence question is whether some edge-case came in the back door. I think the best evidence against that possibility: validate() throws an exception if a declaration has an empty() name. (And other things would probably emit warnings.)

• Makes cleanup required and declares explicitly the default value previously implied

The upgrade step goes a little bit further than just filling null values - it coerces all unrecognized values. It's still probably OK. But I'm not clear why that part is there. I'm guessing it's a general aesthetic desire for strictness? (Note: If there was some hypothetical extension feeding unrecognized values for cleanup, then I don't see how an upgrade-step would be sufficient. You need different step to filter whenever the extension is activated anew.) If it's just a general aesthetic thing then I probably would've pursued that via validate().

OTOH, I don't think anyone is quite crazy enough to put hacked values into cleanup, so I don't really object. (But this assumption could be wrong. And if there are real cases, then maybe that changes things.)

---

Anyway, the patch generally makes sense. I did a DB upgrade (dmaster, 5.66=>master), and schema came out as expected. The main risk is that there are unforeseen use-cases for the standardized data (in which case the standardization would be destructive). I can't point to any, so I don't think we should do anything hard. Regardless, it seems cheap+safe to take snapshot beforehand, so I pushed that bit up.

[colemanw]

Thanks @totten

The upgrade step goes a little bit further than just filling null values - it coerces all unrecognized values. It's still probably OK. But I'm not clear why that part is there. I'm guessing it's a general aesthetic desire for strictness?

I was just matching the policy already set by the business logic:

civicrm-core/CRM/Core/ManagedEntities.php

Lines 388 to 389 in 4f2060b

	default:
	throw new CRM_Core_Exception('Unrecognized cleanup policy: ' . $policy);

This way if the field contains an empty string it will be coerced to 'always' which is what happens already at runtime:

civicrm-core/CRM/Core/ManagedEntities.php

Line 359 in 4f2060b

$policy = empty($item['cleanup']) ? 'always' : $item['cleanup'];