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

FIX Use installer 5.3 with graphql 5.2 #117

Open
wants to merge 1 commit into
base: 1.17
Choose a base branch
from

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Jan 16, 2025

Issue silverstripe/.github#357

Fixes issue where elemental 5.3.4 is being used because framework 5.2 is being used as installer 5.2 is the lowest version compatible with graphql version in gha-generate-matrix causing CI to fail

Instead we need elemental 5.3.5 which requires framework 5.3

We need to use framework 5.3 in order to get this fix

After multiple attempts at doing this with composer, e.g. this, it seems like the only way to actually do this is to hardcode it straight into gha-generate-matrix

@GuySartorelli
Copy link
Member

The last thing we discussed, you thought this could be resolved by changing a const. I'm guessing that wasn't going to work - can you please explain why? Hardcoding stuff like this is really a last resort so I wanna make sure we've looked at and understood other options first.

@emteknetnz
Copy link
Member Author

I think it may be techincally wrong change the const because it's essentially an auto-generated map of composer dependencies for silverstripe/installer. It might have some unintended side-effect that I'm not aware of.

'5.2' => [
  ...
  'silverstripe-graphql' => '5.1', // changed from 5.2
],
'5.3' => [
  ...
  'silverstripe-graphql' => '5.2', // remains the same
],

If we did that we'd still need to put a big inline comment explaining why it was done. I think we're better off doing the big explainer in code instead.

@GuySartorelli
Copy link
Member

GuySartorelli commented Jan 16, 2025

it's essentially an auto-generated map of composer dependencies for silverstripe/installer.

It's only auto-generated because we got sick of doing it manually every minor update. But if that map is wrong, we should correct it.

If we did that we'd still need to put a big inline comment explaining why it was done.

We need that comment either way - I'd be much happier with a comment in the const rather than a big block of hardcoded stuff in the code that is immediately not useful to us after a couple of releases.

I also don't like the precedent of hardcoding stuff like this - remember how bad the travis config ended up with all of its hardcoded overrides?

If updating the const to what is now currently correct works, I think that's better than essentially overriding the const value in code with a block of hardcoded stuff.

@emteknetnz
Copy link
Member Author

emteknetnz commented Jan 16, 2025

But if that map is wrong, we should correct it.

The map is right though, installer 5.2 is supposed to use graphql 5.2, not graphql 5.1 which is what I'd need to change it to.

Actually I'm actually not even sure if changing the const it would even work, as you clearly cannot actually install graphql 5.1 with installer 5.2. If we were to do this would be better to change it to

'5.2' => [
  ...
  'silverstripe-graphql' => null, // big comment explaining why we're doing this
],

and hope that nothing else breaks

I think we're far better off doing this in code

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.

2 participants