Follow fix to PR #130 for the remaining setup/runtime issues I hit while testing this branch.#131
Follow fix to PR #130 for the remaining setup/runtime issues I hit while testing this branch.#131ThingEngineer wants to merge 3 commits intoOpen-SGF:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses remaining onboarding/runtime friction after #130 by aligning the local Docker/PHP platform with the dependency set, making setup scripts safe to run on the host before Sail/DB are available, and removing generated Laravel cache artifacts that should not be tracked.
Changes:
- Update Sail runtime to PHP 8.4 and align Composer’s PHP platform requirement accordingly.
- Add a
composer setupscript to clear config and clear cache using a host-safe store. - Remove tracked generated files from
bootstrap/cache/*and adjust onboarding instructions in the README.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yml | Switch Sail build context/image tag to the PHP 8.4 variant. |
| composer.json | Require PHP ^8.4 and add a setup composer script for config/cache clearing. |
| bootstrap/cache/services.php | Remove generated/compiled cache file from version control. |
| bootstrap/cache/routes-v7.php | Remove generated/compiled cache file from version control. |
| bootstrap/cache/packages.php | Remove generated/compiled cache file from version control. |
| bootstrap/cache/events.php | Remove generated/compiled cache file from version control. |
| bootstrap/cache/config.php | Remove generated/compiled cache file from version control. |
| app/Services/PdfIntakeFormService.php | Adjust PDF template path resolution used during PDF generation. |
| app/Services/Integrations/NeonApiService.php | Make Neon config nullable and add a configuration guard before API calls. |
| app/Console/Commands/PollNeonParticipants.php | Add additional logging for polled participant counts. |
| README.md | Update first-run command order and document PHP 8.4 + composer setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function getTodaysParticipantIds(): array { | ||
| $todaysDate = Carbon::today('America/Chicago')->format('Y-m-d'); | ||
| // $todaysDate = '2026-02-24'; | ||
| // $todaysDate = '2024-02-24'; // for testing | ||
| Log::info("Collecting participant records that have been added or updated today - {$todaysDate}...."); |
There was a problem hiding this comment.
ensureConfigured() is only called from fetch(), but getTodaysParticipantIds()/getParticipantIdsByDate() (the path used by neon:poll-participants) builds URLs and makes HTTP calls using $this->baseUrl/$this->apiKey without invoking ensureConfigured(). If those config values are unset, this will fail later with a less clear error (e.g., invalid URL) rather than the intended configuration exception. Consider calling ensureConfigured() at the start of getTodaysParticipantIds() (or inside getParticipantIdsByDate() and other public methods that make requests).
|
|
||
| // Load and fill the PDF | ||
| $pdf = new Pdf(storage_path("app/{$this->pdfTemplatePath}")); | ||
| $pdf = new Pdf(storage_path("{$this->pdfTemplatePath}")); |
There was a problem hiding this comment.
$pdfTemplatePath is set to intake-form/..., but the template in this repo lives under storage/pdfs/intake-form/... (see PdfFillFields using storage_path('pdfs/intake-form/...')). With storage_path("{$this->pdfTemplatePath}") the resolved path becomes storage/intake-form/..., which won’t exist and will cause PDF generation to fail. Consider either updating $pdfTemplatePath to include the pdfs/ prefix (or using storage_path('pdfs/'.$this->pdfTemplatePath)), or using Storage::path() with the correct disk-relative path.
| $pdf = new Pdf(storage_path("{$this->pdfTemplatePath}")); | |
| $pdf = new Pdf(storage_path('pdfs/'.$this->pdfTemplatePath)); |
|
Let me know if any of these changes are out of line but I can now cleanly and fully start this project on a fresh clone now. I also noticed I created this branch from the wrong target branch for PR #130 rather than main so my commits sat on top of pr/jugglingdev/130. I did a rebase --onto to lift just my commits off the PR #130 base and plant them on main. So this PR can be considered and merged after #130. |
ThingEngineer
force-pushed
the
josh/setup-follow-up
branch
from
3899929 to
d546294
Compare
- composer setup is now host-safe and idempotent for cache/config clearing
ThingEngineer
force-pushed
the
josh/setup-follow-up
branch
from
d546294 to
e2761f8
Compare
jugglingdev
left a comment
jugglingdev
left a comment
There was a problem hiding this comment.
Lots of these changes look great and are super helpful. Thank you for testing and catching these, Josh!
Just one note I have is that I don't think we have anything to seed for the enrollment bot project. We can verify that with the team.
| - Duplicate the .env.example: `cp .env.example .env` | ||
| - Start the project containers: `sail up -d` | ||
| - Generate a new APP_KEY: `sail artisan key:generate`. This will automatically update the .env file for the APP_KEY value. | ||
| - Run initial DB migration scripts: `sail artisan migrate --seed` |
There was a problem hiding this comment.
Do we have anything to seed for the enrollment bot?
This PR fixes several lingering onboarding issues found while testing #130. composer setup was failing on the host before Sail was running because cache:clear tries to connect to the Docker-hosted MySQL instance via the mysql hostname, which isn't resolvable from the host; switching to cache:clear file makes the command host-safe and idempotent across setup states (before and after Sail and DB availability). The README's first-run command order is also updated to make clear that Sail needs to be running before migrations. docker-compose.yml and composer.json are aligned to PHP 8.4, which is already required by the locked Symfony 8 dependencies. Without this, containerized composer and artisan commands fail on platform checks. Finally, generated cache files are removed from version control since the .gitignore in that folder already excludes them, so they should not have been tracked.