pkp/pkp-lib#8975 Fixed argument reflection handling

[jonasraoni]

No description provided.

[Vitaliy-1]

On the top of my head, Mailables can support intersection types but not union types, except cases, like in ORCID Plugin, where classes belong to different applications (but nevertheless, I'd check if it works as expected)

Constructor arguments are identified before Mailable instantiation to forecast which email template variables are going to be available when sending an email. E.g., say class1 is a associated with email template variable1 and class2 with variable2, then using the union

public function __construct(class1|class2 $object)

would mean that both template variables, 1 and 2, are available, which isn't true.

As a workaround we can ensure that both class names in the union don't exist in the application at the same time and exclude the one that isn't defined. Or forbid union types and deal with it on the plugin side. Or maybe there is a cleaner way...

[jonasraoni]

Ops, I forgot to answer 😅

I thought about this case, and it will work out of the box for our main case Server|Journal|Preprint (only one of the classes will exist, and the is_a() will skip the other variants properly).

[jonasraoni]

For the other cases (where the class might actually exist), like let's say an email that might receive a Failure|Success... I think we have the following options:

• A. Build a separate email class for each variant
  This is the simplest for us and users, so that would be my choice. It will become unmaintainable just if we had many of such "intersection" variables in a single email, but then we can revisit this topic.
• B. Add "Smarty" capabilities to the template, so the users can setup loops/conditions/etc (We {if Failure)failed{else}succeeded{/if})
  This would require adding a "view code" / building a powerful and intuitive editor 😅

[jonasraoni]

So, if you don't have anything against, I think it's ok to merge it this way, and use the option A 🤔

[jonasraoni]

And for future updates, I think that maybe it's better to have a method/interface, like errrrrr, public static function getVariables(), then we can call it and the class will still have a chance to customize its variables. It will release the constructor to be used for other things (e.g. it might make sense to pass a Journal when building the email, but perhaps I don't want it to be part of the variables).

Take my comments with a grain of salt as I've just skimmed fast through the code =]

[Vitaliy-1]

@jonasraoni, thanks for double checking that it works. Can you also add an exception to unsure that both classes in the union don't exist at the same time? It's easy to forget it, especially for someone who haven't worked with mailables before.

[jonasraoni]

When there's more than one active class in the union type, I've updated the code to retrieve only the first class and emit a warning.
Anyway, I think it's worth to open an issue to revisit how these variables are retrieved later.

[Vitaliy-1]

Thanks, @jonasraoni. I left a comment but feel free to merge as is if you don't have a time for it right now, in this case please open an issue to not forget to return to it after the release.

[Vitaliy-1]

@jonasraoni, thanks for double checking that it works. Can you also add an exception to unsure that both classes in the union don't exist at the same time? It's easy to forget it, especially for someone who haven't worked with mailables before.