Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(REF) Remove _tagElement dynamic property #25265

Merged
merged 1 commit into from
Jan 2, 2023

Conversation

braders
Copy link
Contributor

@braders braders commented Jan 2, 2023

Overview

The $_tagElement property was a bit odd. Look at how it was used:

foreach ($this->_tags as $tagID => $tagName) {
  $this->_tagElement = &$this->addElement('checkbox', "tag[$tagID]", NULL, $tagName);
}

Issues with this code:

  1. The _tagElement is being re-written on each iteration of the loop. So $this->_tagElement will point to the last element of the loop. I can't see how that is useful for anyone? If it was $this->_tagElement[] = ... then maybe; but it's not
  2. The &$this shows how old this code is. If I understand correctly this was required in PHP4 when objects were not passed as reference by default. I checked the hisory of one file and the property was added when the functionality was first developed, not as part of a later ticket.
  3. $this->_tagElement is a dynamic property, which is deprecated in PHP 8.2.
  4. _tagElement is written, but never read or used in core.

We could add a @deprecated annotation to $_tagElement first, but based on the above I think it's unlikely third-parties are using _tagElement and I suspect it's safe to just remove.

CRM/Contact/Form/Search.php did specify $_tagElement explicitly, but as the task classes don't extend CRM_Contact_Form_Search that doesn't make any sense...

With this change no references to _tagElement remain in core.

@civibot civibot bot added the master label Jan 2, 2023
@civibot
Copy link

civibot bot commented Jan 2, 2023

(Standard links)

@demeritcowboy demeritcowboy merged commit f5a88dd into civicrm:master Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants