Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.As a follow up to
Annotation based plugins don't need a use statement anymore and #2090353: Don't require to put the use statement into plugin classes, I offer this patch to remove all the 'use' statements for Annotation classes in the core plugins.
Yeah, it's big, but it only affects a few lines at the top of each file so the potential for conflicts is small.
| Comment | File | Size | Author |
|---|---|---|---|
| #35 | rebase-monday.patch | 237.01 KB | TR |
| #27 | remove-use-annotation-statement-rebase-27.patch | 236.45 KB | TR |
| #25 | remove-use-annotation-statement-rebase-25.patch | 236.98 KB | TR |
| #23 | remove-use-annotation-statement-rebase-23.patch | 238.06 KB | TR |
| #21 | remove-use-annotation-statement-rebase-21.patch | 238.52 KB | TR |
Comments
Comment #2
TR CreditAttribution: TR commentedRebased to current HEAD
Comment #3
dawehnerI really like that diffstat:
Sadly I could still find a couple of left use statements:
Comment #4
TR CreditAttribution: TR commentedYes, I only attempted to fix the 'use' statements in the core modules.
Part of the reason I did this was as a compromise: We can either fix this on a module-by-module basis (like #2094003: Remove use statements in formatters, widgets and field types plugins started to do), which imo is very inefficient and will take forever, or we can try to do it in big chunks like this patch.
But I'm concerned that already the patch is too big to have a chance of getting committed in a timely manner.
If you think a larger patch which includes those additional 100 files is preferable, I can roll the larger patch. Or we can commit this one and then do one more round of clean up to address everything outside of core/modules/*/lib.
Comment #5
TR CreditAttribution: TR commentedSetting back to needs review to get some more feedback on the correct course of action for this patch.
Comment #6
TR CreditAttribution: TR commentedHere's a alternative patch; this one removes the 'use' statements from all of core.
Some notes:
I left in the
use Drupal\Component\Annotation\Plugin;for classes that extend Plugin.I left in the
use Drupal\Component\Annotation\AnnotationInterface;for classes that implement AnnotationInterface.Even though this may not be strictly necessary, I think in these cases it's very important to be explicit about, e.g., *which* "Plugin" class is being subclassed. For documentation, if nothing else.
Also, I removed four lines in the doxygen comments for the core/lib/Drupal/Core/Annotation/Translation.php class that instruct the reader to always
use Drupal\Core\Annotation\Translation;. Because obviously we don't want them to do that.And for those of you who are interested:
421 files changed, 0 insertions(+), 749 deletions(-)Comment #8
dawehner6: remove-use-annotation-statement-complete.patch queued for re-testing.
Comment #10
TR CreditAttribution: TR commentedFailure is a testbot failure, not a patch failure: "Detect a Drupal installation failure"
There's something going on with the testbot right now. It took 13 hours for the test to run (all tests in the core queue were marked "Postponed" for those 13 hours) and now the tests are failing to run.
When the testbot is working properly again I'll retest the patch.
Comment #11
TR CreditAttribution: TR commented6: remove-use-annotation-statement-complete.patch queued for re-testing.
Comment #13
TR CreditAttribution: TR commented2: remove-use-annotation-statements-2.patch queued for re-testing.
Retesting the first patch to test if it's the testbot or perhaps the second patch that's causing the Drupal installation failure.
Comment #14
TR CreditAttribution: TR commentedTurns out the test fail was my fault, not the testbot's. The patch in #6 removed two 'use' statements which shouldn't have been removed, causing an installation failure. And for the nth time, I'm thankful we have tests to catch this sort of thing.
Here's a new patch. Only difference is that two fewer 'use' statements were removed.
Comment #15
dawehnerNice, it passed!
Comment #16
longwaveComment #17
TR CreditAttribution: TR commentedRebased against current HEAD.
I'd appreciate some movement on this issue - because of the size it's going to be difficult to keep this patch applicable to HEAD.
Comment #19
TR CreditAttribution: TR commentedAnother rebase.
Comment #20
TR CreditAttribution: TR commentedResetting the status based on #15 and #16 - patch has only been rebased to keep it applying to HEAD, no other changes since the two reviews.
Comment #21
TR CreditAttribution: TR commentedYet another rebase in a seemingly vain attempt to keep up with core changes ...
All this patch does is remove some 'use' statements, tests still run green without them.
EDIT: 2 tests failed. One fail was a testbot failure - out-of-memory error. The other was in a module not touched by these changes and was because D8 HEAD was broken at the time.
Comment #23
TR CreditAttribution: TR commentedHEAD keeps moving.
Comment #24
dawehnerLet's try this time.
Comment #25
TR CreditAttribution: TR commentedAnother rebase to keep pace with HEAD development.
Comment #27
TR CreditAttribution: TR commentedOK, this is getting ridiculous. Patch in #25 failed because a core file was removed in the space of time it took me to pull the new D8, rebase the patch, then post the new patch.
Perhaps I should file 406 different issues, one for each file that I'm changing? Otherwise it's WAY too much effort to keep re-rolling this. I'm leaving this at "RTBC" because there is no additional content to the patch since it was RTBC in #15 and #16 - it's just rebasing to keep up with file name changes and file removals.
Comment #28
webchickThis is a "normal" issue. It's simple clean-up, not blocking anything else. Core committers (rightly) spend their time on critical/major issues, as well as blocking issues, prior to spending time on refactoring patches like this. It's nothing to get upset about. It means the system is working. It's much better to break patches like this than critical/major bug fixes that get D8.0 to release faster.
Probably what makes the most sense here is to pick a day to do this and other types of "this is going to break tons of more important patches in the queue" and get them all in at once, similar to "SunDXay" we did the other month. Generally weekends are best, because activity is down. Is there a particular time that works best for you, TR?
Comment #29
webchickMaking up a tag to track this type of wide-scale refactoring. (as a corollary to the "Avoid commit conflicts" tag)
Comment #30
TR CreditAttribution: TR commentedI wasn't criticizing you webchick - you're doing a crazy amount of work. I know this issue is not high priority. It is a fragile patch and a lot of work because of the far-reaching scope of #2090353: Don't require to put the use statement into plugin classes, and it's frustrating to see other equally minor patches be submitted after this and get committed before this.
I don't mind doing the work, I just don't want to waste time I could be using for something else. So any suggestions you have for how to get things like this accomplished without a putting in a huge excess of work are appreciated.
My suggestion would be that issues like #2090353: Don't require to put the use statement into plugin classes need to fix core as part of the issue - i.e. this patch should have been part of the commit for that issue. I feel that way about change notices too - the issue summary should provide a complete change notice that should be committed as part of the issue, otherwise change notices lag way behind the core changes, which makes keeping up with D8 HEAD difficult (things break, no change notice to be found or way too sketchy, and issue queue searches still don't work since the site upgrade). longwave and I are trying to keep our 84,000 lines of D8 Ubercart code running against D8 HEAD while we're re-architecting the internals, so these suggestions are based on my experience porting and maintaining non-trivial code in the current D8 environment. If D8.0 is going to be developer-ready, these things have to take place in parallel with the core changes, not as afterthoughts.
Another suggestion is that patches that are "minor" should get *less* scrutiny, because they are much easier to revert if there's a problem. Documentation patches and the like.
Anyway, that's pretty far afield from the topic of this issue. I'm trying to keep the patch up-to-date for now, but I can't promise I'll do that forever.
Comment #32
TR CreditAttribution: TR commented27: remove-use-annotation-statement-rebase-27.patch queued for re-testing.
Comment #33
webchickHm. The only other patch(es) along this lines are all the "unused variables" ones which I committed a rash of on Tuesday, just to finally get them out of my face. But previously a lot of them been just been sitting at RTBC for 3+ weeks, for the same reason I haven't prioritized this issue. I also tagged the meta issue for that as "Will cause commit conflicts," because you're right, it's very much the same: minor clean-up that potentially has far-reaching breakage. Fair point about committing patches like this along with the initial change itself, though. And I *do* really appreciate patches based on real-world contrib work in D8.
All I literally meant though was, let's pick a date, and then hold off on re-rolling efforts until then, so you don't waste your time, and I don't feel guilty. :) The next D8 alpha is due on Dec 16. How about we get this in right after, on late Dec 16/early Dec 17? Would that work for you?
Comment #34
TR CreditAttribution: TR commentedSure, that works fine. Thanks. I'll make sure to have a rebased patch testing green waiting for you here next Monday.
Comment #35
TR CreditAttribution: TR commentedRebased as of today. Let's see what the testbot says.
Comment #36
webchickThanks, TR! Alpha7 still hasn't gone out (I think catch was busy today, but assigning this to me so I can get it in right after). If this patch ends up breaking a critical I might need to revert so we may go back and forth a couple of times, but hopefully we can get it done early this week. :) Thanks for your patience!
Comment #37
webchickAll right! Alpha7 is out and this still miraculously applied! :)
Committed and pushed to 8.x. Woohoo! Thanks, TR, for this nice DX clean-up.