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

Don't install symfony/console via composer #636

Merged
merged 1 commit into from Feb 22, 2020

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Feb 20, 2020

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb force-pushed the ignore-symfony-console-dependency branch from f6c428c to ab054f9 Compare February 20, 2020 20:49
@kesselb
Copy link
Contributor Author

kesselb commented Feb 20, 2020

Not sure about the lock file update 😕

@Grotax Grotax merged commit 96aba4b into master Feb 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the ignore-symfony-console-dependency branch February 22, 2020 18:22
@Grotax Grotax mentioned this pull request Mar 12, 2020
Grotax added a commit that referenced this pull request Mar 18, 2020
Changed
- Basic Media-RSS support (#599)
- Database index improvements (#637)

Fixed
- Call to a member function getUrlHash() on null" when adding a feed (#640)
- Don't install symfony/console via composer (#636)
- Fix for for ONLY_FULL_GROUP_BY (see #406) (Issue #80) (#407)
- Catch invalid feeds (#646)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@@ -59,7 +59,8 @@
"symfony/service-contracts": "1.1.8"
},
"replace": {
"guzzlehttp/guzzle": "*"
"guzzlehttp/guzzle": "*",
"symfony/console": "*"
Copy link
Member

Choose a reason for hiding this comment

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

I've never user replace but from https://getcomposer.org/doc/04-schema.md#replace it sounds a bit dangerous. Like you're telling composer that this package replaces guzzle and symfony/console. And I assume your intention is that the app shall fine and use the ones from server, right? So this can work if the composer here assumes the same guzzle version as in Nextcloud, but might fail hard otherwise, right? Like here composer assumes guzzle x is available, but in reality version y is shipped.

Copy link
Member

Choose a reason for hiding this comment

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

if the libs here can also work with httplug instead of guzzle directly, you may also try https://packagist.org/packages/christophwurst/nextcloud-http-client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Using replace is tricky. symfony/console is luckily only a false positive. feed-io does not depend on symfony/console (actually it does but only for some cli scripts not used by news). guzzlehttp/guzzle maintains a stable api for a while. It seems that all guzzle versions shipped from Nextcloud 16 to Nextcloud 19 do work with feed-io.

Copy link
Member

Choose a reason for hiding this comment

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

guzzlehttp/guzzle maintains a stable api for a while. It seems that all guzzle versions shipped from Nextcloud 16 to Nextcloud 19 do work with feed-io.

I've seen issues with Guzzle in twofactor_gateway when we shipped our own version. It was caused by _idn_uri_convert or similar. It was somewhat related to guzzle/guzzle#2600 (comment) but I do not remember the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

occ files_external:delete on external smb storage fails with php fatal error Breaks occ -h
4 participants