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

WIP - Refactor to cater for falsey get_plugin() return value #46

Closed

Conversation

davegreenwp
Copy link
Contributor

@davegreenwp davegreenwp commented Nov 29, 2023

Fixes: #45

This PR refactors any instance of get_plugin to handle the fact that this could return boolean false, thanks to interference from 3rd party plugins. Unit tests have been updated to cater for the possible false return value.

These changes only deal with the problem as it presents, the exact source of the problem is still unclear, and I'm waiting for a customer to provide access to a staging site so that I can identify the plugin causing the problem on production.

@davegreenwp davegreenwp requested a review from borkweb November 29, 2023 11:18
@davegreenwp davegreenwp self-assigned this Nov 29, 2023
Rollback license field changes.

Check for falsey rather than is_array

Drop Resource type declaration

License field refactor

Additional false handling

Modified unit test
@davegreenwp davegreenwp force-pushed the fix/45-unxpected-return-value-current-method branch from 86cc1a6 to 2b69182 Compare November 29, 2023 12:01
@borkweb
Copy link
Member

borkweb commented Nov 29, 2023

@davegreenwp - I see what you are aiming to accomplish with this PR, however, returning bool from the Collection is not a desirable path forward for this library - the collection should only return types that it collects.

I suspect that the problem that is being experienced lies within the order of operations that happen in the plugin and the moment that the license field is being output. If the collection is returning null for the plugin, then that indicates the problem likely lies with the order of operations. The plugin needs to register itself within Uplink before the license fields are called.

@davegreenwp
Copy link
Contributor Author

No longer required as #49 is the correct fix for the underlying issue.

@davegreenwp davegreenwp closed this Dec 1, 2023
@defunctl defunctl deleted the fix/45-unxpected-return-value-current-method branch December 1, 2023 15:14
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.

Fatal error due to unexpected return value in Collection::current()
2 participants