-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Generate helpers after package discovery #1185
base: master
Are you sure you want to change the base?
Conversation
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.
Makes sense to me and completes your prior work, thanks @netpok !
I've added the changelog entry and also updated the readme, I think it makes sense to promote this now. What do you think?
Yeah, that's the idea, sorry for the late answer. Can I do anything on my side to expedite merging? |
@netpok maybe syncing with master (or rebase if you prefer), since the last change is already > 1 month old. There are no merge conflicts, but I guess it's good to sync. Would be great if you could be so kind and then ping me once more, thank you |
@mfn Rebased it to master |
Hmm, it seems the prefer-lowest have an issue resolving dependencies, same happened in #1216 (comment) |
Have not yet analyzed it, but FTR I'm pro removing this one anyway, I don't think it's that useful (see #1076 ) |
I couldn't reproduce it locally with this json (the one in CI is modified during the run): {
"name": "barryvdh/laravel-ide-helper",
"description": "Laravel IDE Helper, generates correct PHPDocs for all Facade classes, to improve auto-completion.",
"keywords": [
"laravel",
"autocomplete",
"ide",
"helper",
"phpstorm",
"netbeans",
"sublime",
"codeintel",
"phpdoc"
],
"license": "MIT",
"authors": [
{
"name": "Barry vd. Heuvel",
"email": "[email protected]"
}
],
"require": {
"php": "^7.3 || ^8.0",
"ext-json": "*",
"barryvdh/reflection-docblock": "^2.0.6",
"composer/composer": "^1.6 || ^2",
"doctrine/dbal": "^2.6 || ^3",
"illuminate/console": "^8",
"illuminate/filesystem": "^8",
"illuminate/support": "^8",
"laravel/framework": "8.*",
"nikic/php-parser": "^4.7",
"phpdocumentor/type-resolver": "^1.1.0"
},
"require-dev": {
"ext-pdo_sqlite": "*",
"illuminate/config": "^8",
"illuminate/view": "^8",
"mockery/mockery": "^1.4",
"orchestra/testbench": "^6",
"phpunit/phpunit": "^8.5 || ^9",
"spatie/phpunit-snapshot-assertions": "^3 || ^4"
},
"suggest": {
"illuminate/events": "Required for automatic helper generation (^6|^7|^8)."
},
"config": {
"sort-packages": true
},
"extra": {
"branch-alias": {
"dev-master": "2.9-dev"
},
"laravel": {
"providers": [
"Barryvdh\\LaravelIdeHelper\\IdeHelperServiceProvider"
]
}
},
"autoload": {
"psr-4": {
"Barryvdh\\LaravelIdeHelper\\": "src"
}
},
"autoload-dev": {
"psr-4": {
"Barryvdh\\LaravelIdeHelper\\Tests\\": "tests"
}
},
"minimum-stability": "dev",
"prefer-stable": true,
"scripts": {
"analyze": "psalm",
"check-style": [
"php-cs-fixer fix --diff --diff-format=udiff --dry-run",
"php-cs-fixer fix --diff --diff-format=udiff --dry-run --config=.php_cs.tests.php"
],
"fix-style": [
"php-cs-fixer fix",
"php-cs-fixer fix --config=.php_cs.tests.php"
],
"psalm-set-baseline": "psalm --set-baseline=psalm-baseline.xml",
"test": "phpunit",
"test-ci": "phpunit -d --without-creating-snapshots",
"test-regenerate": "phpunit -d --update-snapshots"
}
} Full output of time composer update -vvv --prefer-lowest --prefer-dist
Probably the interesting part:
Maybe the environment on CI is so much slower that it simply exceeds the timeout. We could submit this to composer but TBH, I find the chase for |
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. This is a re-submit of barryvdh#1076 See also: - barryvdh#1216 (comment) - barryvdh#1185 (comment)
I've submitted (i.e. re-submitted my previous attempt) for consideration: #1217 |
I would be happy to use this feature if possible, is there a blocking reason @barryvdh ? |
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. This is a re-submit of #1076 See also: - #1216 (comment) - #1185 (comment)
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. This is a re-submit of barryvdh/laravel-ide-helper#1076 See also: - barryvdh/laravel-ide-helper#1216 (comment) - barryvdh/laravel-ide-helper#1185 (comment)
I would be happy to use this feature if possible, is there a blocking reason @barryvdh? |
@netpok hmm, I guess it was just forgotten 😞 And we were side tracked back then with some unrelated errors popping up. I've approved it once and see no reason to retract :) Can you please rebase with current master (there's a merge conflict) and we need to make sure all tests are 🟢 |
@mfn I rebased 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.
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. This is a re-submit of barryvdh/laravel-ide-helper#1076 See also: - barryvdh/laravel-ide-helper#1216 (comment) - barryvdh/laravel-ide-helper#1185 (comment)
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. This is a re-submit of barryvdh/laravel-ide-helper#1076 See also: - barryvdh/laravel-ide-helper#1216 (comment) - barryvdh/laravel-ide-helper#1185 (comment)
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. This is a re-submit of barryvdh/laravel-ide-helper#1076 See also: - barryvdh/laravel-ide-helper#1216 (comment) - barryvdh/laravel-ide-helper#1185 (comment)
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. This is a re-submit of barryvdh/laravel-ide-helper#1076 See also: - barryvdh/laravel-ide-helper#1216 (comment) - barryvdh/laravel-ide-helper#1185 (comment)
IMHO it's impractical to expect this package to have to work with the "first release of Laravel for any major release" even when there are already sever more releases: no one is/should stick to that version. This is a re-submit of barryvdh#1076 See also: - barryvdh#1216 (comment) - barryvdh#1185 (comment)
Summary
This PR allows automatic helper file generation after composer install or update.
This is accomplished by running the set commands after
package:discover
successfully completed. This command is executed as part of the dump-autoload script and technically we want to generate the helpers after packages are discovered so this should be even semantically okay-ish.Originally I thought this would be unnecessary but as it turns out there is no way to run ide-helper in a way that it does not break in non-dev environment without introducing a userland command under Windows.
The only caveat is that
php artisan package:dis
will still execute the package discover command but won't trigger this automation, since the main target is to run after composer commands this should not matter much.This should also make PR #1067 unnecessary.
Type of change
Checklist
composer fix-style