[Do not merge] [NFC] dev/core#1116 - document where activityTypeName is name and where it's label

[demeritcowboy]

Overview

See also #14952. Documents good/bad usage and adds test.

Before

Mishmash of misnamed variables.

After

Still there but pulled out for testing so at least one of them doesn't flip back and forth.

Technical Details

All I changed was to add comments and move the block verbatim into its own function for testing. I checked that the local variables aren't used outside that block, which is a nice surprise.

Comments

As noted in the lab ticket for the form member variable _activityTypeName it's not desired to remove or change the variable since it might be used in form hooks.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@demeritcowboy so the way I think we can go forwards is to leave the properties misnamed but name the getters & setters correctly - ie change your public function assignActivityTypeName to setActivityTypeLabel & add a getter getActivityTypeLabel & we can migrate the code to using the functions & not referencing the property directly

[demeritcowboy]

Ok I can take a hint - he he. You're probably wondering how long this will go on and when I will start doing something. I think I do have a good enough sense now of the scope.

I completely agree with abstracting it into functions and as a temporary thing I see no problem with calling this particular function somethingLabel, just I think it's a missed opportunity if it were to be left like that too long and starts getting re-used. I disagree with that naming because part of this 10-year ongoing problem is that the words "name" and "label" are too similar and easy to mix up, so if I'm going to change the wording at all then I'm thinking here and now would be one place to remove that confusion.

I also can see I'm going to need the real name as well in the same place in order to fix where it should be name, and that pattern comes up often where both are needed, so again if I'm going to change it at all then it makes sense to me to set/return BOTH at the same time not just label.

And I see a slight side benefit to always setting/returning both since it adds a small layer of human-proofing against accidentally requesting the wrong one. Yes you could still use the wrong one, but you would at least see that there are two. It's maybe a small layer, but is at least another layer, like at the airport where they have that person who checks your boarding pass for what seems like no reason right before going into the other line where they also check your boarding pass.

Maybe you're not convinced, and are going to say the answer is to have more tests, and of course that would help either way, but I'm thinking it would be a missed opportunity to do more.

[eileenmcnaughton]

@demeritcowboy of course tests would help :-)

I do think code cleanup is one of those things where you have to feel your way into it & sometimes you start down one path & then something else makes sense -but as long as you are still making small incremental cleanups you are moving in the right direction - so we don't need to nail everything now.

OTOH I probably would make sure the new function you are adding here 'does what it says on the packet' - I guess it's hard because it assigns label as name - perhaps it makes sense to add to it & also assign label in the same function & then start to switch over tpl instances.

I'm OK to merge this if you want & let you keep going as I think that IS the nature of working through cleanup

[demeritcowboy]

Ok thanks appreciate the feedback - I'll update the PR tomorrow - am doing mostly non-civi stuff today. I'd almost forgotten there is such a thing.

[eileenmcnaughton]

Lol - yeah that thing called a life....

[demeritcowboy]

Will probably end up closing this one just leaving here for reference for the moment. Have a series of smaller steps starting with #14999.