Php8.2 Contact import Map Field screen - remove undefined property usage (mutliple) #25298

eileenmcnaughton · 2023-01-09T19:27:02Z

Overview

Removes rather than declares _columnCount (https://github.com/civicrm/civicrm-core/pull/25296/files#diff-4f0a1c0d1f7a02c448ebe273bd5b951a1811dd14949b11b9c8914b2c198fceba was going with declare

This is a legacy property

Before

We can see the assign is never used (only in search related forms)

We can see that _columnCount is only still used in the Contact MapField form - the others simply loop through the headers (rather than counting them & the using that number in a for loop)

civicrm-core/CRM/Activity/Import/Form/MapField.php

Line 86 in 76b53f2

foreach ($columnHeaders as $i => $columnHeader) {

Unused variable

_dataValues is only used for an unused function

After

Note there are still 7 rows - that is what the columnCount was used to display

Technical Details

Comments

civibot · 2023-01-09T19:27:05Z

(Standard links)

If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2023-01-09T22:52:10Z

I did an r-run on this & also decided to fix some of the notices I encoutered at the smarty level

#25302

totten · 2023-01-11T02:46:00Z

CRM/Contact/Import/Form/MapField.php

@@ -174,12 +171,12 @@ public function buildQuickForm() {
        $id = $first = $second = NULL;
      }
      if (($first === 'a' && $second === 'b') || ($first === 'b' && $second === 'a')) {
-        $cType = $contactRelationCache[$id]["contact_type_{$second}"];
+        $cType = $contactRelationCache[$id]["contact_type_$second"];


Subjective aside: I find this old expression more readable than this new expression. (With the new one, my eyes have to re-scan a few times to confirm that $ really is there and recognize that second is a variable-name.)

Granted, Github UI and PHPStorm UI render this differently. PHPStorm's highlighter changes the color of $second (which serves a similar purpose of helping you recognize the variable-name). I suppose that (within PHPStorm) the {}s are visually redundant. Maybe that's why they nag you to remove them.

But other highlighters don't work the same way. The change makes the code less readable in other environments.

PHPStorm's nag feels counter-productive to me. It can be fixed, though:

Of course, both forms are legal, and it doesn't affect the mergability of this PR.

@totten - yes I generally figure that when it comes to preferences I'd rather go with what has been generally agreed elsewhere rather than come up with our our negotiated version of what is more readable. (My own readability preference in this case would be to concatenate it onto the end :-)).

as an aside - I've seen several things that phpstorm encouraged in the main inspections & the Extended ones become required as we've gone up the php versions

on the readability I look at that line & think - WTF is 'cType' so I don't get to the quoted part before my brain refuses to parse it so that's an 'unscannable' line from my POV

Since I wound up looking at this I decided to fix the variable names & then I wound up extracting the determination of the value - as is usually the case in civi those curly braces in a string point to a bit of code that is just nasty

on the readability I look at that line & think - WTF is 'cType' so I don't get to the quoted part before my brain refuses to parse it so that's an 'unscannable' line from my POV

Heh. Yeah. cType probably felt natural to whoever wrote it (at the time). Of course, if I look at that out-of-context, then I think: "oh, C language type -- void* or char**! Wait, why are we using C types in PHP?" 🙃

... I wound up extracting the determination of the value ...

If you're looking at some code and find some change which (subjectively) seems easier to read/understand, then power to you. 🚀

And yes - I was right - phpstorm IS signalling an upcoming deprecation https://php.watch/versions/8.2/$%7Bvar%7D-string-interpolation-deprecated

Just noting that this was incorrect, and it seems sort of material to topic. Both php.watch and the original RFC say that {$var} is preferred over ${var} (since the latter is duplicative and less general).

But the language benefits from having some flavor of {} , because bare $var can be ambiguous. I haven't seen any authority say {$var} is going away.

I'd rather go with what has been generally agreed elsewhere rather than come up with our our negotiated version of what is more readable

Let's take that and treat it as a question. What is the generally agreed approach from elsewhere vis-a-vis standards for variables-in-strings? Is it:

"foo_$bar"

"foo_{$bar}"

'foo_' . $bar

Other/None of the above

For "elsewhere", we can look to large communities like drupal.org, wordpress.org, php-fig.org, and symfony.com. They all use PHP variables. They all have coding standards. But none of them offers an answer to this question. (There do address some narrower cases -- like t() for Drupal prose and sprintf() for Symfony error-messages -- but this isn't prose. There's nothing that applies to this kind of string.)

Why not? Are Drupal developers too shy to haggle over obscure details? Is this a newfangled problem that coding-scientists have never seen before? No... I submit that the absence points to answer (4). They don't standardize this because it's not worth doing. (The cost of standardizing exceeds the benefit.)

It seems to me that PHPStorm is the outlier, and... even so, PHPStorm is rather non-committal (since it puts the "Disable" option right there in the popup...).

(To be clear -- I'm not saying that we should ignore everything PHPStorm says. I'm saying we should ignore it when it's wrong. 😈)

@totten I think the 'disable option right there in the popup' is a bit of a red herring - that option is there for all inspections.

But since you seem to care about this one I'll disable it. I go with phpstorm because it is a standard, picks up lots of good stuff - some of which have since been enforced, and helps us avoid bike shedding the details. But in terms of readability I think using actual words is much more important in this code than how the string is concatenated

eileenmcnaughton · 2023-01-11T03:24:02Z

These are real test fails - I'll take a look

Test Result (9 failures / +8)CRM_Contact_Import_Form_MapFieldTest.testLoadSavedMappingDirectCRM_Contact_Import_Form_MapFieldTest.testDefaultFromColumnNames with data set #0CRM_Contact_Import_Form_MapFieldTest.testDefaultFromColumnNames with data set #1CRM_Contact_Import_Form_MapFieldTest.testDefaultFromColumnNames with data set #2CRM_Contact_Import_Form_MapFieldTest.testDefaultFromColumnNames with data set #3CRM_Contact_Import_Form_MapFieldTest.testDefaultFromColumnNames with data set #4CRM_Contact_Import_Form_MapFieldTest.testDefaultFromColumnNames with data set #5CRM_Contact_Import_Form_MapFieldTest.testDefaultFromColumnNames with data set #6

I did a grep for this but it is also worth noting the reason this is no longer used is the parser has a function to get it from the submitted values & we don't pass the form around anymore

…port) form Use strict comparison for cType (cat type?) cType is always a word so strict is good

eileenmcnaughton · 2023-01-11T04:37:36Z

I just checked the testDefault tests and put up a fix - it's a bit arguable

The test checks we guess at the default from the column header. The test uses an sql datasource not a csv - I'm not terribly sure that is valid - but .... I can use the equivalent of the former code to make it continue to work per the test

eileenmcnaughton · 2023-01-12T00:50:44Z

test this please

seamuslee001 · 2023-01-12T02:24:55Z

Jenkins re test this please

eileenmcnaughton · 2023-01-12T22:31:11Z

test this please

eileenmcnaughton · 2023-01-17T21:51:37Z

test this please

colemanw · 2023-01-18T15:37:37Z

Code cleanup makes sense and this is well tested.

civibot bot added the master label Jan 9, 2023

eileenmcnaughton changed the title ~~Php8.2 Contact import - remove columnCount~~ Php8.2 Contact import - remove undefined property usage _columnCount, _contactSubType Jan 9, 2023

eileenmcnaughton changed the title ~~Php8.2 Contact import - remove undefined property usage _columnCount, _contactSubType~~ Php8.2 Contact import Map Field screen - remove undefined property usage _columnCount, _contactSubType Jan 9, 2023

eileenmcnaughton mentioned this pull request Jan 9, 2023

[REF][PHP8.2] Further fixes for resolving deprecations on Dynamic Cla… #25296

Closed

eileenmcnaughton force-pushed the import branch from 6369589 to 20c122d Compare January 9, 2023 21:57

eileenmcnaughton changed the title ~~Php8.2 Contact import Map Field screen - remove undefined property usage _columnCount, _contactSubType~~ Php8.2 Contact import Map Field screen - remove undefined property usage (mutliple) Jan 9, 2023

eileenmcnaughton force-pushed the import branch from ea6aaa6 to d080105 Compare January 9, 2023 23:14

totten reviewed Jan 11, 2023

View reviewed changes

eileenmcnaughton added 10 commits January 11, 2023 17:16

Php8.2 Contact import - remove columnCount

8d31ec5

Remove _columnCount from Contact MapField

5a8206b

Fixes to type hints & docblocks

1ea5d3b

Cleanup type hints and comments

430c546

Remove undeclared & unused property

922acb3

I did a grep for this but it is also worth noting the reason this is no longer used is the parser has a function to get it from the submitted values & we don't pass the form around anymore

NFC remove unnecessary curly braces

9b4e908

Remove legacy paramter columnNames

315bb36

Remove usages of dataValues, + unused function that calls it

3310b21

Remove another undefined property - LocationType

5522f85

Remove last dynamic property (_contactType) from MapField (Contact im…

59adc04

…port) form Use strict comparison for cType (cat type?) cType is always a word so strict is good

eileenmcnaughton force-pushed the import branch from d080105 to 59adc04 Compare January 11, 2023 04:35

eileenmcnaughton force-pushed the import branch from 6b9651c to 59adc04 Compare January 11, 2023 05:05

Use meaningful variable names

402c5c5

eileenmcnaughton force-pushed the import branch from 26a0378 to 4ac6fda Compare January 11, 2023 19:19

Simplify / clarify contact type simplification

bdcf5fd

eileenmcnaughton force-pushed the import branch from 4ac6fda to bdcf5fd Compare January 11, 2023 19:19

colemanw merged commit a1c3dbf into civicrm:master Jan 18, 2023

colemanw deleted the import branch January 18, 2023 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Php8.2 Contact import Map Field screen - remove undefined property usage (mutliple) #25298

Php8.2 Contact import Map Field screen - remove undefined property usage (mutliple) #25298

eileenmcnaughton commented Jan 9, 2023 •

edited

Loading

civibot bot commented Jan 9, 2023

eileenmcnaughton commented Jan 9, 2023

totten Jan 11, 2023

eileenmcnaughton Jan 11, 2023

eileenmcnaughton Jan 11, 2023

eileenmcnaughton Jan 11, 2023

eileenmcnaughton Jan 11, 2023

totten Jan 11, 2023 •

edited

Loading

eileenmcnaughton Jan 11, 2023

eileenmcnaughton commented Jan 11, 2023

eileenmcnaughton commented Jan 11, 2023

eileenmcnaughton commented Jan 12, 2023

seamuslee001 commented Jan 12, 2023

eileenmcnaughton commented Jan 12, 2023

eileenmcnaughton commented Jan 17, 2023

colemanw commented Jan 18, 2023

Php8.2 Contact import Map Field screen - remove undefined property usage (mutliple) #25298

Php8.2 Contact import Map Field screen - remove undefined property usage (mutliple) #25298

Conversation

eileenmcnaughton commented Jan 9, 2023 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jan 9, 2023

eileenmcnaughton commented Jan 9, 2023

totten Jan 11, 2023

Choose a reason for hiding this comment

eileenmcnaughton Jan 11, 2023

Choose a reason for hiding this comment

eileenmcnaughton Jan 11, 2023

Choose a reason for hiding this comment

eileenmcnaughton Jan 11, 2023

Choose a reason for hiding this comment

eileenmcnaughton Jan 11, 2023

Choose a reason for hiding this comment

totten Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton Jan 11, 2023

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 11, 2023

eileenmcnaughton commented Jan 11, 2023

eileenmcnaughton commented Jan 12, 2023

seamuslee001 commented Jan 12, 2023

eileenmcnaughton commented Jan 12, 2023

eileenmcnaughton commented Jan 17, 2023

colemanw commented Jan 18, 2023

eileenmcnaughton commented Jan 9, 2023 •

edited

Loading

totten Jan 11, 2023 •

edited

Loading