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

[2.x] modConnectorResponse — allow dynamic properties #16562

Conversation

marcuspoehls
Copy link

What does it do?

Added #[\AllowDynamicProperties] PHP attribute to the modConnectorResponse.

Why is it needed?

This change silences a deprecation warning in PHP 8.2 because of dynamic property assignments in this class. This also fixes issues with the package management, because the response now contains valid JSON instead of HTML.

Related issue(s)/PR(s)

@opengeek
Copy link
Member

What dynamic properties are being assigned to this class? It sounds like the source of the issue may need to be addressed rather than solving it with the AllowDynamicProperties attribute.

@marcuspoehls
Copy link
Author

@opengeek
Copy link
Member

The dynamic property is $response, assigned here: https://github.com/modxcms/revolution/blob/45e4b239f72a2352e788e4f2a3c3f0f456a5e175/core/model/modx/modconnectorresponse.class.php#L144C14-L144C28

Thanks. That seems like an oversight. I'll dig in today or tomorrow and see if I can't resolve this by defining the expected properties.

@smg6511
Copy link
Collaborator

smg6511 commented Apr 22, 2024

I believe all that needs to be done is to add $response = null; to the base modResponse class props. The same goes for 3.x.

@opengeek
Copy link
Member

I believe all that needs to be done is to add $response = null; to the base modResponse class props. The same goes for 3.x.

I'm not sure that's the only property being dynamically assigned here. Also, these should likely be private properties, but I'll need to investigate.

@marcuspoehls
Copy link
Author

@opengeek Thank you Jason for looking into this 🙂

@opengeek
Copy link
Member

See #16563 for my proposed solution to this warning. If you agree with the solution, please close this PR, @marcuspoehls.

@marcuspoehls
Copy link
Author

@opengeek Hey Jason, thank you for your pull request that refines the actual issue with the dynamic $response property. I’m closing this pull request in favor of yours #16563

@marcuspoehls marcuspoehls deleted the 2.x-modconnectorresponse-allow-dynamic-properties branch May 12, 2024 03:51
opengeek added a commit that referenced this pull request Oct 30, 2024
#16563)

### What does it do?
Assign the processor result to a local variable rather than dynamically
creating a "response" property on the modConnectorResponse class.

### Why is it needed?
In PHP 8.2, dynamically assigning a property to a class is deprecated.
When calling runProcessor, the result is only used within the
`outputContent()` method, so there is no need to assign the result from
runProcessor to a property of modConnectorResponse dynamically or add a
property to modConnectorResponse to hold the result.

### How to test
Confirm no deprecated warnings are logged when running a processor in a
connector.

### Related issue(s)/PR(s)
Alternative solution to #16562
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants