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.

Sign up for GitHub

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

Jump to bottom

Upgrade cs lib #1069

Merged
merged 10 commits into from
Nov 8, 2020
Merged

Upgrade cs lib #1069

merged 10 commits into from
Nov 8, 2020

Conversation

greg0ire
Copy link
Member

No description provided.

@greg0ire greg0ire force-pushed the upgrade-cs-lib branch 5 times, most recently from 6323d5f to a6fd240 Compare October 28, 2020 20:02
@greg0ire greg0ire force-pushed the upgrade-cs-lib branch 7 times, most recently from 626fbd7 to b4359cc Compare October 28, 2020 22:23
@greg0ire greg0ire mentioned this pull request Oct 28, 2020
Closed
@greg0ire
Copy link
Member Author

The Travis build fails because of an issue I reported here: box-project/box#489

@greg0ire greg0ire mentioned this pull request Oct 30, 2020
Closed
@greg0ire greg0ire force-pushed the upgrade-cs-lib branch 3 times, most recently from d3935c1 to a7e1335 Compare October 30, 2020 08:01
@greg0ire
Copy link
Member Author

Blocked by box-project/box#490

goetas
goetas approved these changes Oct 31, 2020
View reviewed changes
Copy link
Member

@goetas goetas left a comment

Choose a reason for hiding this comment

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

I am good with this changes. They improve the current state of the codebase.

goetas
goetas previously requested changes Oct 31, 2020
View reviewed changes
Copy link
Member

@goetas goetas left a comment

Choose a reason for hiding this comment

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

there is a 404 to download the box

@greg0ire greg0ire marked this pull request as draft October 31, 2020 10:51
@greg0ire
Copy link
Member Author

Converting to draft until the numerous issues with box are resolved.

@greg0ire greg0ire requested a review from goetas November 7, 2020 10:11
@greg0ire greg0ire marked this pull request as ready for review November 7, 2020 10:11
@greg0ire
Copy link
Member Author

greg0ire commented Nov 8, 2020

I just noticed that the PHAR compilation was only ever tested with low dependencies. With 96ce53a, it's not tested at all, and I don't understand if testing it only once was deliberate. @jwage , maybe you remember? I believe you changed this in #700

@greg0ire
Copy link
Member Author

greg0ire commented Nov 8, 2020

And of course now it fails with the same errors I had when attempting to migrate to GA… that had nothing to do with GA.

@greg0ire
Copy link
Member Author

greg0ire commented Nov 8, 2020 •
edited
Loading

Good news: I'm reproducing the failure locally.

@greg0ire
Copy link
Member Author

greg0ire commented Nov 8, 2020

Ah apparently this is supposed to be fixed by 3.9.0: box-project/box@e65178c … which cannot be downloaded as a PHAR : box-project/box#490

@goetas
Copy link
Member

goetas commented Nov 8, 2020

Ah apparently this is supposed to be fixed by 3.9.0: box-project/box@e65178c … which cannot be downloaded as a PHAR : box-project/box#490
😢

@greg0ire
Use a more recent (and less buggy?) version of box
19ece86 
3.1.0 crashes with syntax error, unexpected 'fn' (T_FN), expecting
  identifier (T_STRING)
@greg0ire
Update some of the packages
f90109d
@greg0ire
Do not use /tmp as a migration directory
a0f5ae5 
You do not know what it may contain. If it contains directories
unreadable for the current user, for instance, the test suite will crash
because these cannot be opened.
@greg0ire
Copy link
Member Author

greg0ire commented Nov 8, 2020

A version of the PHAR was just manually uploaded! It fixes the build locally, I just pushed 🎉

@greg0ire greg0ire force-pushed the upgrade-cs-lib branch 2 times, most recently from 88dc534 to ff4bae1 Compare November 8, 2020 14:26
@greg0ire
Apply new dependencies policy
e7dde1e 
We no longer put composer.lock under version control and resort to
composer install. Instead, if a tool breaks often, we use tighter
version constraints for it.
@greg0ire greg0ire dismissed goetas’s stale review November 8, 2020 15:34

The review has been addressed

goetas
goetas approved these changes Nov 8, 2020
View reviewed changes
Copy link
Member

@goetas goetas left a comment

Choose a reason for hiding this comment

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

👍 looks good!

@greg0ire greg0ire merged commit f918b95 into doctrine:2.3.x Nov 8, 2020
@greg0ire greg0ire added this to the 2.3.1 milestone Dec 5, 2020
@github-actions github-actions bot mentioned this pull request Dec 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoench smoench smoench left review comments

@goetas goetas goetas approved these changes

@beberlei beberlei Awaiting requested review from beberlei

@SenseException SenseException Awaiting requested review from SenseException

Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
2.3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@greg0ire @goetas @smoench