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][PHP8.2] Avoid dynamic properties in mailstores #25243

Merged

Conversation

braders
Copy link
Contributor

@braders braders commented Dec 30, 2022

Overview

Avoid dynamic properties in mailstores. See https://lab.civicrm.org/dev/core/-/issues/3833 for context.

Before

_mailbox, _ignored, _processed, _dir and _leftToProcess were declared dynamically, which will be deprecated in PHP 8.2.

After

The properties are explicitly declared.

Comments

I've declared the properties as private/protected. It's possible that something is trying to read one of the above properties in an extension, but I think it's unlikely - the mailer object is not passed into any hooks, which means any extension would struggle to get a refrerence to the mailer object in order to read it's properties.

That said, happy to change the properties to public if it would make people feel more comfortable with merging this.

@civibot
Copy link

civibot bot commented Dec 30, 2022

(Standard links)

@civibot civibot bot added the master label Dec 30, 2022
@eileenmcnaughton
Copy link
Contributor

I'm not seeing universe references to MailStore other than @artfulrobot's patchwork - which seems to use this as an example so I think a ping is enough & we can go with the protected / private

image

@eileenmcnaughton eileenmcnaughton merged commit 77b8cc9 into civicrm:master Dec 30, 2022
@artfulrobot
Copy link
Contributor

@eileenmcnaughton Thanks for the shout out, this change should not affect https://github.com/artfulrobot/mailstorepermissions (and, ironically, if it did then my extension could easily undo it anyway as it patches this file!)

@eileenmcnaughton
Copy link
Contributor

@artfulrobot proof of concept!

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.

3 participants