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

Enhancements required for linkfield migration #11171

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Mar 6, 2024

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

Issue

@kinglozzer
Copy link
Member

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
Copy link
Member Author

GuySartorelli commented Mar 7, 2024

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.

Comment on lines 38 to 42
/**
* Used by unit tests to override `isCli()`
* @internal
*/
private static ?bool $cliOverride = null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* Used by unit tests to override `isCli()`
* @internal
*/
private static ?bool $cliOverride = null;
/**
* Used by unit tests to override `isCli()`
* This is not config, so use reflection to change the value
* @internal
*/
private static ?bool $isCliOverride = null;

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines 260 to 261
if (self::$cliOverride !== null) {
return self::$cliOverride;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (self::$cliOverride !== null) {
return self::$cliOverride;
if (self::$isCliOverride !== null) {
return self::$isCliOverride;

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@GuySartorelli GuySartorelli changed the base branch from 5.2 to 5 March 17, 2024 20:58
@GuySartorelli GuySartorelli changed the title ENH Add lightweight test override for Environment::isCli() Enhancements required for linkfield migration Mar 18, 2024
@GuySartorelli GuySartorelli force-pushed the pulls/5.2/test-non-cli branch from 130aeb2 to 1879bce Compare March 18, 2024 00:50
@GuySartorelli
Copy link
Member Author

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.

@GuySartorelli GuySartorelli force-pushed the pulls/5.2/test-non-cli branch from 1879bce to faab576 Compare March 18, 2024 01:13
@emteknetnz emteknetnz merged commit 25f6114 into silverstripe:5 Mar 19, 2024
15 checks passed
@emteknetnz emteknetnz deleted the pulls/5.2/test-non-cli branch March 19, 2024 22:50
GuySartorelli added a commit to beerbohmdo/silverstripe-framework that referenced this pull request Apr 18, 2024
* ENH Add lightweight test override for Environment::isCli()

* NEW Allow JOIN with SQL UPDATE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants