[phpMyAdmin Git] [phpmyadmin/phpmyadmin] 22a6fc: Minor refactoring of InsertEdit.php (#17199)

Kamil Tekiela tekiela246 at gmail.com
Fri Nov 26 18:19:07 CET 2021


Branch: refs/heads/master
Home: https://github.com/phpmyadmin/phpmyadmin
Commit: 22a6fc9045635907f581975d2c9e86aa328b5395
https://github.com/phpmyadmin/phpmyadmin/commit/22a6fc9045635907f581975d2c9e86aa328b5395
Author: Kamil Tekiela <tekiela246 at gmail.com>
Date: 2021-11-26 (Fri, 11 November 2021) -03:00

Changed paths: 
M libraries/classes/Controllers/Table/ReplaceController.php
M libraries/classes/InsertEdit.php
M phpstan-baseline.neon
M psalm-baseline.xml
M test/classes/InsertEditTest.php

Log Message:
-----------
Minor refactoring of InsertEdit.php (#17199)

* Simplify condition

isset is not needed since the variable is set and we check explicitely for an array so it cannot be null either.
!empty is not needed because the variable is set, so we just need to check if it has non-empty value

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Reduce indentation thanks to early returns

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Collapse isset check

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove redundant variable assignments

$specialCharsEncoded is immediately overwritten in if/else.
$noSupportTypes is always empty and never used in this code.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove redundant !empty() checks on parameters and defined variables

if(!empty($var)) on defined variables is redundant and equivalent to if($var)

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove redundant ternary

$dispval is declared to be a string so the ternary is redundant

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Refactor showEmptyResultMessageOrSetUniqueCondition

Replace boolean variable with returns, which leads to redundant else
block. Remove redundant unset, which leads to redundant variable assignment.
The return statement can be replaced with a boolean cast.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove redundant unset at the end of the method

When a variable leaves scope it is automatically unset

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* [RISKY] Remove temporary var and error supression

It's risky because I was not able to determine the purpose of the error
suppression. In case of error, the function seems to return -1, but that
has not been checked here either. It was possible to silence undefined
global variable, but in that case, why was it not done only on that one
line?

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove redundant condition

The variable is declared as int|false so if it is not false and not 0,
the only logical choice is a non-zero integer, which is always true.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Misleading type hint leading to invalid unit tests

According to DbiMysqli the first argument should be mysqli_result.
The correct type hint cannot be array. It can be object though.
Tested with a debugger that the passed value is really mysqli_result.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove dead code

Remove $inputType. I left the comemnt for posterity, but the variable is
dead. If in the future someone decides to implement it again, then
introducing the variable will not be a problem.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Move block of code to where it should be

I am not sure about this block of code. I don't fully understand what
it is supposed to do. However, making the query at the start of the
function doesn't make sense to me.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Use array_keys instead of foreach with dummy variable

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Use is_null instead of isset

True conditions are easier to read than inverted conditions.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove redundant variable and break

When the loop breaks then the variable is true.
When the loop finishes then the variable is false.
We can simply replace it with return true/false to make it simpler.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Add type declarations

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Refactor getEnumSetAndTimestampColumns

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove redundant condition

According to searchColumnInForeigners $foreigner is dependant on values
from $foreigners which makes this check redundant.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Refactor getColumnEnumValues

Simplify parameter list, add psalm types, and simplify code

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Refactor getColumnSetValueAndSelectSize

Unfortunately, I cannot type hint the return value as then I would have
to type-hint the param as well.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove ternary operator

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Refactor getSelectOptionForUpload

Added types, removed NULL return (it is only outputted in Twig as
HTML string so NULL made no sense), and simpified parameters.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Refactor getMaxUploadSize

According to getMaxUploadSize the value can only be int.
Comparisons of numerical strings should be avoided. Psalm return type
can be added with the exact types.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Refactor getColumnSize

It takes a string as a second parameter now.
The reason for this is that during normal execution, the value comes
from column specifier e.g. VARCHAR(255), which is a string. The value
in spec_in_brackets could also represent ENUM/SET values, so it can't be
forced as an int.
This change required changing tests to pass a string instead of an array
with an int.
The behaviour should remain the same thanks to the (int) cast (previously
implicit, now explicit).

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove parameter $realNullValue

This parameter was always false. Instead define it as false inside
each method. The methods are private, so this should not cause any problems.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Add psalm-return

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Add an early return and clarify return type

This function should never return false

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Change return type to multidimensional array

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Refactor getCommentsMap

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Type hint $tableColumns as multidimensional array

This simplifies static analysis a little bit

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Update phpstan baseline

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Fix invalid unit test

The method getTableColumns should return a multidimensional array always.
Added proper type hint to avoid further mistakes and changed mock.
According to my analysis this method should always return a
multidimensional array because parameter $column to getColumns() is null.
Also, this test is pretty useless as it just seems to test the bahaviour
of array_values which behaves as an identity function right now.

Note: The DBAL needs urgent refactoring too. I imagine array_values is
redundant at the moment. The getColumns should only return a list or
columns or a single column. There's no need to reindex the list. I think
Psalm should have picked it up, but I didn't check.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove redundant call to Util::extractColumnSpec()

Warning! There is no coverage in unit tests for this piece of code.
While I have analysed the code flow manually, it is possible the
duplicated call had some unknown to me purpose.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Update psalm-baseline.xml

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Adhere to the coding standard

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Revert "Remove ternary operator"

This reverts commit 3cbec2cebb9efcc7d52988a5b5720c0e43dc6ddb.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Rename $field_MD5 to $fieldHashMd5

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Exclude string '0' from comparison and invert the order of operation

This helps with readability and it's unlikely that we would want to
check for string '0' here.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Rename $spec_in_brackets to $specInBrackets

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove else statements

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Introduce array_key_exists() and a local variable

We also add phpdoc comment telling static analysis that the value should
be a string. This is what the current code expects already.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>

* Remove object parameter type declaration

Upon further inspection I realize this was wrong. The unit tests rely
on the parameter type being an integer and so we cannot force it to be
only an object. I leave the type hint as object as array was probably
never the right type.

Signed-off-by: Kamil Tekiela <tekiela246 at gmail.com>



More information about the Git mailing list