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

Config Only: "php: < 7" #9683

Open
sminnee opened this issue Sep 11, 2020 · 3 comments
Open

Config Only: "php: < 7" #9683

sminnee opened this issue Sep 11, 2020 · 3 comments

Comments

@sminnee
Copy link
Member

sminnee commented Sep 11, 2020

The config system allows for conditional config on environment types, environment variables, module presence, classes existing, and PHP extensions being loaded. (This is implemented in CoreConfigFactory.php)

It would be valuable to add config that is conditionally applied when PHP version is less than or greater than a given value.

One key use case is to allow the continued support of older versions of PHP by retaining "legacy implementations" of services, rather than a bunch of conditional code in the service.

There are couple of of approaches to take.

Syntax A

First is that the argument takes a version number and is always a >= comparison:

---
Name: new-class
Only:
  php: 7.2
---
SilverStripe\Core\Injector\Injector:
  MyService:
    class: NewServiceImplementation
---
Name: old-class
Except:
  php: 7.2
---
SilverStripe\Core\Injector\Injector:
  MyService:
    class: LegacyServiceImplementation

Syntax B

The other approach is that we allow inequalities

---
Name: new-class
Only:
  php: >= 7.2
---
SilverStripe\Core\Injector\Injector:
  MyService:
    class: NewServiceImplementation
---
Name: old-class
Only:
  php: < 7.2
---
SilverStripe\Core\Injector\Injector:
  MyService:
    class: LegacyServiceImplementation

Personally I'd go with the first option as it's more consistent with the other only/except flags, which don't have any support for comparison operators.

Looking at the code in CoreConfigFactory.php, this should be quite straightforward to implement.

Does the feature make sense to people? Do you prefer Syntax A or B?

@ScopeyNZ
Copy link
Member

ScopeyNZ commented Sep 11, 2020

I like it, but I also prefer syntax B, as most changes will apply to an arbitrary range of versions. I think the comparison operators aren't inconsistent here as none of the other "rules" are numeric. I played around a little with trying to tie some regex into version_compare but it's not great:

https://3v4l.org/PnCT9

The 7.2 is basically just read as 7.2.0 by version_compare and then you get unexpected results.

@sminnee
Copy link
Member Author

sminnee commented Sep 11, 2020

Yeah I think you would need to strip the current version so that it's got the same precision as the version in the config. And you'd want a test around that logic of course, to work through these edge cases.

@sminnee
Copy link
Member Author

sminnee commented Sep 16, 2020

Anyway, given Guy's feedback I'll try and pull together a PR for syntax B

@sminnee sminnee self-assigned this Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants