Coding standard changes

could 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).

@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.

my 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.

I 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. :-(

Adding a new patch !

+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.

cd src
grep -Rc 'array(' *.php

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

Putting in needs work for the reroll.

Rerolled the patch

My previous test for completeness was broken, what follows is better.

 find . -name '*.php' -o  -name '*.install' -o -name '*.module' | xargs grep -c '\ array(' | more

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.

Needs reroll. :-(

I 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

Because 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:

1. The change to the "description" text in flag.install. Embedding all those spaces in the string is wrong (and breaks translations incidentally). Breaking the string into sections and concatenating them would also be wrong. It is correct and acceptable in our coding standards to leave the string the way it was. It's important to remember that the coding standards checker *does* generate false positives - a lot of them - so you have to use discretion in making these changes. I just left that change out - I didn't try to fix it.
2. Indentation in hook_requirements() - in your attempt to fix the long line of code you messed up the indentation, now it's 3 characters instead of 4 in some places. And in fact all that code is commented out and the standards checker gives you false indentation warnings because it doesn't expect comments to be indented hierarchically like code is.
3. Changing // '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.
4. Reformatting of the buildForm() declaration in src/Form/FlagConfirmFormBase.php. Unnecessary, not required by coding standards, and inconsistent with the style of all our other code. I left this out.
5. In several files, several places, the patch removes blank lines that were there to separate blocks of inline comments from each other. That changes the meaning of the comments and makes them less clear. The proper way to do this and avoid the coding standards warning is to use /* */ style comment blocks and KEEP the blank lines, rather than use // style. I left all these changes out.
6. In several files, several places, indentation was altered and made wrong. I left those changes out.
7. ACK! In flag.token.inc, you changed all the t() to $this->t(). What's $this in procedural code? No, this is quite wrong and will totally fail.

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.

@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.

In the future I recommend fixing one thing at a time

Yep, we should do that in future.

I have created a meta issue so we can sort things out one at a time.

That looks good to me too!