-
Notifications
You must be signed in to change notification settings - Fork 0
ETT-634 Use composer rather than downloading phar files #122
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
base: main
Are you sure you want to change the base?
Conversation
…h need for messing around with the include path. - We should have most of PEAR taken care of. - `test/RecordUtilsTest.php` was having trouble with the `global $htstatus` that PHPUnit wants to set to null. - PHPUnit vendored (and slightly version bumped, although it is still a couple major versions old).
- Fix two warnings/errors in HTID.php exposed by playwright tests.
…ry to only invoke it when needed. - Will this get in the way by running too often?
| header('Content-type: text/html; charset=UTF-8'); | ||
| echo "<h1>Not found</h1>"; | ||
| echo "'$id' is not a valid record identifier."; | ||
| echo "'$htid' is not a valid record identifier."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug exposed by playwright tests
|
|
||
|
|
||
| header("Location: /Record/$id", true, '301 Moved Permanently'); | ||
| header("Location: /Record/$id", true, 301); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug exposed by playwright tests
|
|
||
| require_once 'vendor/autoload.php'; | ||
| require_once 'sys/LoggingPager.php'; | ||
| require_once 'Pager/Pager.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Pager/Pager.php PEAR or a built-in? May be able to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's PEAR plus our own subclass Pager_LoggingPager that is indeed used, but I don't know what it brings to the table vs the base PEAR class. It doesn't seem to actually do any logging.
|
|
||
|
|
||
| $HT_COLLECTIONS = eval(file_get_contents('__DIR__/../derived_data/ht_collections.php')); | ||
| global $htstatus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phpunit has a reputation for not playing nicely with globals. These tests apparently broke because we updated the version slightly with composer. This two-line declare/assign is needed to keep the variable from becoming null inside the individual tests.
| profiles: | ||
| - playwright | ||
|
|
||
| composer: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run this before another of the PHP services (vufind, phpunit) to make sure the dependencies are there.
aelkiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tried this out yet but just looking at what's here:
- Doing the
composer installin this way via docker compose looks pretty slick and could be an approach we take with thebundle installin ruby-land. - Generally with bundler in ruby we've used a separate volume in the docker compose rather than installing them in the working directory. There are pros and cons either way. If we add them in the working directory we need to make sure the vendor directory is ignored in
.dockerignore. - As far as deployment, I would probably expect we'd do the
composer installin test.catalog as part of staging, then deploy everything.
|
Another thought is that there are a bunch of Debian packages we're installing ( |
liseli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started the Catalog application locally, and all the features are working smoothly. I'll go ahead and approve it.
composerwith other PHP dependenciescomposer.jsonandcomposer.lock(it's a big one!) are our Gemfile analogues.require_once 'vendor/autoload.php'vendor/(à labundle install) should not need to be done manually since I have added a lightweightcomposerservice as a dependency forvufindandphpunitin the docker compose. This should work inside thebabelproject without modification, I hope?composer installdoes not wipe-and-reinstall ifvendor/matches the data incomposer.lock, making it a near-noop. If so then this approach should not be burdensome so long as it's reliable.which composerreturns nothing)phpunitbut it does not appear to be a zero-effort upgrade path, so out of scope.Note: do not merge until we have
composeravailable on the relevant VMs