tempest-framework
tempest-framework copied to clipboard
tempestphp
feat(console): add ability to publish vendor files
This pull request adds the ability to interactively publish vendor files to the user's codebase.
This works by adding a new CanBePublished attribute, which gets discovered by a new PublishDiscovery. The new publish:install commands then enables the user to chose which files they want published.
Things to do:
- [x] Add a default target path for published files (depends on #519)
- [x] Update the namespace of the published class (depends on #531)
- [x] Remove the
CanBePublishedandDoNotDiscoverattributes upon publishing (depends on #531) - [ ] Write tests
Related to #509
I'm distracted for half an hour and you already have a PR in place??? How cool is that! Lemme give it a look!
Add a default target path for published files. Currently, users are required to type the target path by hand. I haven't done this because there's no easy way right now to determine the main namespace of the user's codebase.
Maybe the default should simply be the app/src folder? Since Tempest doesn't really impose a project structure, I figure the project root makes most sense, people can drag it from there to wherever they want.
@brendt I'm distracted for half an hour and you already have a PR in place??? How cool is that! Lemme give it a look!
I felt inspired 😎
@brendt Maybe the default should simply be the app/src folder? Since Tempest doesn't really impose a project structure, I figure the project root makes most sense, people can drag it from there to wherever they want.
Absolutely! There's no util for me to determine easily whether the user uses app, src or something else, is what I meant. This is easily solvable though, that specific piece of code exists in the discovery implementation.
Another thing, it seems that the default argument isn't taken into account with the ask console method—looks like a bug
Another thing, it seems that the default argument isn't taken into account with the ask console method—looks like a bug
Could be, I'm fine if you want to fix it right within this branch, otherwise we can take it in a separate PR if you don't feel like it.
@brendt Could be, I'm fine if you want to fix it right within this branch, otherwise we can take it in a separate PR if you don't feel like it.
I created another pull request to keep the commit history clean: #518
@brendt This is now ready for review 👍
I didn't write tests because the interactive parts of the console that are used in PublishCommand are not testable right now.
I'll also PR some changes to the migration manager because it forces the creation of the migration table—which makes me wonder if this should be a publishable class, or not (but we should discuss that in the other PR)
Am I right in understanding we'd have no way to publish views, static assets (js, css, img), etc.?
If so, to me it seems like we might be better off developing some form of package manifest that can be discovered.
@aidan-casey Am I right in understanding we'd have no way to publish views, static assets (js, css, img), etc.?
In this state, absolutely. It only supports PHP classes with valid syntax.
@aidan-casey If so, to me it seems like we might be better off developing some form of package manifest that can be discovered.
We'd still need to manipulate extracted PHP classes, at least for their namespace. If we do that, we fall back to the stub approach...
I'm thinking I should move publishClasses from Kernel to a new PublishConfig, which would have publishClasses and publishFiles. This way, we can keep the #[CanBePublished] approach for PHP classes, and support other files for third-parties (or for Tempest itself, if at some point we publish other assets).
I'm thinking I should move publishClasses from Kernel to a new PublishConfig, which would have publishClasses and publishFiles. This way, we can keep the #[CanBePublished] approach for PHP classes, and support other files for third-parties (or for Tempest itself, if at some point we publish other assets).
Yes, I think I like this approach. I think it'll need to be part of this PR though, because publishing view files will be an important one
Marking this pull request as draft until we resolve the publishing story for all files: https://discord.com/channels/1236153076688359495/1292767408158933032
Just added support for publishing files. Here's how this all works now:
- There's a new
PublishConfigthat holds arrays for classes and files that can be published - The
PublishCommandreads that, and for each file or class, prompts the user which and where to publish them - I had to add support for key/value pairs on the
MultipleChoiceComponentso files could be distinguished from classes in the prompt (https://github.com/tempestphp/tempest-framework/pull/513/commits/bc2e79cb6d2202202295ae196044a6b4507b88e2)
https://github.com/user-attachments/assets/013a85ef-bdb8-4221-9c0c-7f783e27f2c9
The CLI is a bit clunky though. 😵💫
The CLI is a bit clunky though. 😵💫
Do you mean the cancel line that's overwritten partially? Yeah I need to look into that… Made an issue for it: https://github.com/tempestphp/tempest-framework/issues/574
Apart from that: one thing I think we need is an optional search bar for the question component 😁 Actually, there already is a search component but it only supports a single option. I think adding a search flag to ask would be good. Also made an issue for it: https://github.com/tempestphp/tempest-framework/issues/575
PS: I'm not expecting you to do those things, just noting them here for reference :)
Maybe we should also add an option just keep all selected files in their default location, so that you don't have to run through them one by one if you plan to keep everything in the default location.
On top of that (haven't looked at the code yet): did you provide a way to only show files in this list that weren't published yet? Ideally we keep track of which files were published, even if they were moved to another location. I don't know how we'd do that though… Cache? But caches can be cleared. Some kind of local mapping in a file somewhere? But then you'd run into problems if you want to republish a file (although that could be fixed with some kind of --show-all flag ?
Just thinking out loud :) This will be a great feature when finished though!
@innocenzi I made a bugfix for the line clearing issue, could you try it out? https://github.com/tempestphp/tempest-framework/pull/576
It's in main
PS: I'm not expecting you to do those things, just noting them here for reference :)
Honestly, if I was good enough with terminal inputs, I'd already have PR'd a few things—I'd personally love to see features of laravel/prompts built into Tempest!
Maybe we should also add an option just keep all selected files in their default location, so that you don't have to run through them one by one if you plan to keep everything in the default location.
I like the idea. I'll add a flag. Speaking of which, when I wanted to write tests, I realized we don't have a --no-interaction flag 👀
did you provide a way to only show files in this list that weren't published yet?
I thought about it, but couldn't find any solution that was good enough in my book:
- Caching would not work because, as you said, cache can be cleared and wouldn't be shared between developers working on the same repository
- Matching file name or path would not work because files can be renamed
- Matching by file hash wouldn't work either because most published files are published to be modified
So, out of ideas for this one :(
Unsure why these Rector changes don't appear locally—they seem unrelated to this pull request
@brendt aside from the lack of tests, I feel like this is ready for review
Couple of questions:
- Weren't we planning on ditching the
#[CanBePublished]attribute in favour of some kind of package class? - About tests, I'm not sure if you're aware, but
$this->consolewill automatically use the non-interactive version, so you can test these commands. Send me a message if you need more pointers :)
Weren't we planning on ditching the
#[CanBePublished]attribute in favour of some kind of package class?
My understanding was that we should use both—the attribute for PHP classes, and the "package" interface for other files that can't have attributes.
If you'd rather have everything go through that package interface, I'm not sure what that would look like for Tempest core
I remember talking about this on Discord somewhere? But that was before I got sick 😅 I remember I was on the fence about it for a while, but eventually concluded that one consistent approach is more important than two ways of doing the same.
That being said, I'm fine taking it from here if that's ok for you, I think you've done most of the work, and it'll be just a matter of introducing a Package interface with a publishesFiles method, discovering that, and adding those file to the already existing config. So I'll work on this branch to make those changes :)
Pull Request Test Coverage Report for Build 11474462544
Details
- 52 of 140 (37.14%) changed or added relevant lines in 6 files are covered.
- 6 unchanged lines in 3 files lost coverage.
- Overall coverage decreased (-0.7%) to 81.448%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| src/Tempest/Console/src/Config/publish.php | 0 | 1 | 0.0% |
| src/Tempest/Support/src/PathHelper.php | 29 | 31 | 93.55% |
| src/Tempest/Console/src/Commands/PublishCommand.php | 0 | 85 | 0.0% |
| <!-- | Total: | 52 | 140 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| src/Tempest/Auth/src/CreatePermissionsTable.php | 2 | 75.0% |
| src/Tempest/Auth/src/CreateUserPermissionTable.php | 2 | 77.78% |
| src/Tempest/Auth/src/CreateUsersTable.php | 2 | 81.82% |
| <!-- | Total: | 6 |
| Totals | |
|---|---|
| Change from base Build 11471023663: | -0.7% |
| Covered Lines: | 6884 |
| Relevant Lines: | 8452 |
💛 - Coveralls
I gave it a lot more thought:
I argued that #[DoNotDiscover] and #[CanBePublished] shouldn't be attributes, because you can only add them on classes, so you cannot use them on things that aren't classes (view components, config files, sql migrations — did I miss any?)
But here's the thing… all of those edge cases can be solved even with attributes. Let's look at it from a package author's perspective (because that's where these two attributes would be used).
- For migrations, package authors should simply always use classes instead of raw sql migrations. This is already a soft requirement, since you cannot make assumptions about the SQL dialect, hence you'll need the table builder, hence you need a PHP class.
- For view components: we haven't documented this, but package view components should always be namespaced (x-package-button, something like that). It's a convention that we should document, but that means that hiding view components from discovery isn't that big of a requirement. On top of that — same as with migrations — you can make view component classes, which again can use the attribute.
- Config files: well, they aren't discovered anyway, unless your package provides a default .config.php file for them. So that's a non issue.
On top of that, I was looking into this special Package interface, and realised it would have to be yet another exception within discovery (probably a separate kernel boot step), because we'd need to discover all packages to determine which paths should be skipped. This complexity, together with the fact that we might not even really need it, makes me think I favor the attribute approach a lot more now…
As discussed on Discord: we're going to take another approach. Sorry again @innocenzi for all the work you've put into it, but know I really appreciate it, even though we're not merging it!