search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
719 stars 820 forks source link

Enhancements required for linkfield migration #11171

Closed GuySartorelli closed 3 months ago

GuySartorelli commented 3 months ago

Commit 1: Adds a test utility. Required for a test in https://github.com/silverstripe/silverstripe-linkfield/pull/244 Commit 2: Adds JOIN support for SQLUpdate. Required for migrating owners in https://github.com/silverstripe/silverstripe-linkfield/pull/244

Issue

kinglozzer commented 3 months ago

This seems a bit odd - introducing a property with no way to change its value, then using reflection in the linked pull request. Why not add a setter for this? If it’s just for semver reasons, I’d rather we broke that convention than hacked in code like this. If we can’t, then could we introduce a SS_FORCE_IS_CLI environment variable instead of a property/reflection?

GuySartorelli commented 3 months ago

Why not add a setter for this?

This is literally only for use in unit tests. There is no valid use case for using this outside of unit tests, and by not adding a setter usage outside of tests is discouraged.

A setter also becomes public API, which means we have to maintain BC for it afterward, which is not what we want for something that is only for use in testing. Same with environment variables.

GuySartorelli commented 3 months ago

I have added JOIN support for SQLUpdate so I can do an optimisation in the linkfield migration when setting the owner. Since this is targetting the next minor it's safe to do. It also means others can now use this functionality, which is well supported in SQL and really useful. It also will save me at least half a day trying to get kinda-gross workarounds working.