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 Allow installing guzzle 6 compatible version of cow #29

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Jul 7, 2022

Issue #11

Some older branches on modules define guzzle 6 in their composer.json. This change allows the installation of an older version of cow that uses guzzle 6. cow is used for vendor/bin/cow schema:validate in the phplinting test if the module has a .cow.json file

Example of an older version of a module that uses guzzle 6 - https://github.com/silverstripe/silverstripe-ckan-registry/blob/1.4/composer.json#L24

@emteknetnz emteknetnz mentioned this pull request Jul 7, 2022
40 tasks
@GuySartorelli
Copy link
Member

Would it make more sense to change the guzzle constraint in cow master instead?
With this change we risk changing the linting logic in cow and modules not using it because we've allowed some old version to be installed.

I know the intention is that the old version will only be installed if there is a guzzle dependency conflict, but there could be some new conflict in the future that we don't expect which suddenly has a bunch of modules pulling in that old version of cow again for reasons not intended by this change.

@emteknetnz
Copy link
Member Author

Would it make more sense to change the guzzle constraint in cow master instead?

No, it needs to be guzzle 7 to support newer versions of PHP. cow cannot do dual support as dev-master requires "php-http/guzzle7-adapter": "^1", https://github.com/silverstripe/cow/blob/master/composer.json#L28

With this change we risk changing the linting logic in cow and modules not using it because we've allowed some old version to be installed.

That's true, and I don't know of a way around this. However I think the likelihood of changing the linting logic is low. I think we're more likely to get rid of .cow.json files than change their format.

@GuySartorelli
Copy link
Member

We could check the guzzle constraint using composer and use that to determine which version of cow to include... but you're right, it's a pretty unlikely edge case to come up - it would probably be more effort than it's worth to cover it that way.

Maybe we can just create an issue to remove this workaround when we no longer support any versions of modules which need guzzle 6 (which I think will just be when we do the next minor)? I'll merge the PR in the meantime.

@GuySartorelli GuySartorelli merged commit 775ca10 into silverstripe:1.1 Jul 7, 2022
@GuySartorelli GuySartorelli deleted the pulls/1.1/guzzle6cow branch July 7, 2022 22:55
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