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.
coding standard to use short array syntax.
Comment | File | Size | Author |
---|---|---|---|
#15 | 2877902-15.patch | 38.23 KB | TR |
| |||
#12 | 2877902-12.patch | 60.81 KB | rpayanm |
| |||
#9 | 2877902-09.patch | 58.04 KB | harsha012 |
| |||
#6 | flag-array_short_syntax-2877902-6.patch | 23.48 KB | Keenegan |
short-array-syntax.patch | 22.73 KB | sathish.redcrackle | |
Comments
Comment #2
fadgadget CreditAttribution: fadgadget commentedcould this possibly be why my website is down with this message thanks
The website encountered an unexpected error. Please try again later.
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'session_id' in 'where clause': SELECT f.entity_id AS entity_id, f.flag_id AS flag_id, f.global AS global FROM {flagging} f WHERE (entity_type = :db_condition_placeholder_0) AND (entity_id = :db_condition_placeholder_1) AND ((global = :db_condition_placeholder_2) OR ((uid = :db_condition_placeholder_3) AND (session_id = :db_condition_placeholder_4))); Array ( [:db_condition_placeholder_0] => node [:db_condition_placeholder_1] => 978 [:db_condition_placeholder_2] => 1 [:db_condition_placeholder_3] => 0 [:db_condition_placeholder_4] => DlQB7F3kns0WtPyU315IdzvfojE5xA0a-JwuUSseOuI ) in Drupal\flag\Entity\Storage\FlaggingStorage->loadIsFlaggedMultiple() (line 122 of modules/flag/src/Entity/Storage/FlaggingStorage.php).
Comment #3
joachim CreditAttribution: joachim as a volunteer commented@fadgadget Nope, this issue is just a coding style fix. See the release notes for help with your problem.
As for this issue, that's a LOT of files getting changed, and it's going to damage a lot of patches that are waiting for review. I'd rather see this done in smaller chunks, as and when particular parts of the module don't have any patches pending.
Comment #4
fadgadget CreditAttribution: fadgadget commentedmy apologise joachim. i just jumped in and never read up didnt i. i was well warned too. Ill go see other issue as i am having problems uninstalling the module to update to the latest.
Comment #5
socketwench CreditAttribution: socketwench at TEN7 commentedI can clear out the issues that are in need of review -- there aren't that many.
Even so, this patch doesn't quite catch all instances of the old array syntax. I found several even after applying the patch. :-(
Comment #6
KeeneganAdding a new patch !
Comment #7
martin107 CreditAttribution: martin107 as a volunteer commented+1 on the idea behind this issue.
I have had a slow visual scan of the patch .. everything appears appropriate.
BUT The patch needs a reroll.
By way of testing here is a way of asserting that all stuff has been updated.
This is how the codebase looks at the moment.
FlagCountManager.php:3
FlagCountManagerInterface.php:1
FlagInterface.php:0
FlagLinkBuilder.php:0
FlagLinkBuilderInterface.php:0
FlagService.php:4
FlagServiceInterface.php:0
FlagServiceProvider.php:0
FlaggingInterface.php:0
FlaggingViewsData.php:0
Comment #8
socketwench CreditAttribution: socketwench at TEN7 commentedPutting in needs work for the reroll.
Comment #9
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedRerolled the patch
Comment #10
martin107 CreditAttribution: martin107 as a volunteer commentedMy previous test for completeness was broken, what follows is better.
There are some cases of array remaining, specifically only where we have uncommented unmaintained code - I don't think we should fix them here.
Secondly just to be complete ...
This patch alters flag.install ... fixes the indentation in areas of uncommented code surrounding instances of array() but does does not remove the array() themselves!
In short we have out of scope changes .. I am easy going and I would be happy to see these changes be committed on the basis that all the changes look correct.
Comment #11
socketwench CreditAttribution: socketwench at TEN7 commentedNeeds reroll. :-(
Comment #12
rpayanmComment #13
martin107 CreditAttribution: martin107 as a volunteer commentedComment #14
martin107 CreditAttribution: martin107 as a volunteer commentedI think the importance of this issue is increasing
Core has been beavering away fixing and adding more automated coding standard checks.
So the haystack that is all our current coding standard issues gets biggers and it gets harder to discern new errors that a new patch may introduce.
I want to minimise the time need to review this patch .. it takes a long time to do it properly.
My suggestion is that when RTBC we accept all these changes here .. while acknowledging that it does not catch all the new coding standard errors.
We can then grapple with the newly exposed errors in a second smaller issue.
Looking at the patch from #12
I do not think the handling of FlagContextualLinksTest.php is correct.
after applying the patch - it has not been moved correctly ... it is cloned and yet not deleted.
find . -name '*LinksTest.php'
./src/Tests/FlagContextualLinksTest.php
./tests/src/FunctionalJavascript/FlagContextualLinksTest.php
Comment #15
TR CreditAttribution: TR commentedBecause this patch touches much of the code base for the flag module, it's going to break almost every other patch in this queue. Likewise, every other patch is likely to break this coding standards patch.
I think the strategy outlined in #14 is good.
To avoid chasing our tails, this should be committed quickly (assuming nothing is overtly wrong with it) without worrying about completeness. Follow up patches will be more limited in scope and much easier to manager.
In the future I recommend fixing one thing at a time - like changing all uses of array() to [] - rather than trying to fix many different things all at the same time. Keeping the changes focused on one thing makes the patches much easier to review and less likely that some inappropriate things get accidentally mixed in.
I did a very careful line-by-line review of the whole patch, and there are seven things it does that I don't think are proper:
// 'click sortable' => TRUE,
to// 'click sortable' => TRUE.
in two places in flag.views.inc. I assume that was done because the checker told you that comments must end in a period. But this is actually a commented-out line of code, and by changing it you've made it syntactically invalid code so now it won't work if the commenting is removed. Again, this is a false positive and shouldn't be done. I left this change out too.This patch changes a lot of things but misses a lot of things too. That's OK, those can be handled separately without trying to make this as comprehensive as possible. That's why I only removed things rather than adding the many other things that still need to be done.
I also removed the changes to FlagContextualLinksTest.php - in addition to the move there were a LOT of changes that were unrelated to coding standards. No justification was given for the move or the changes, and in fact moving things from PHPUnit tests to Simpletests is the opposite of what we should be doing. The changes look more like changes in progress from another issue than something that should be here. Regardless, since it's not coding standards, it shouldn't be in this patch.
Here's a re-roll of #12 with the above removals.
Comment #16
martin107 CreditAttribution: martin107 as a volunteer commented@TR thank you, for taking the time to document things and make the changes.
I've had a visual scan of the #15
All looks good to me.
Yep, we should do that in future.
I have created a meta issue so we can sort things out one at a time.
Comment #18
socketwench CreditAttribution: socketwench at TEN7 commentedThat looks good to me too!