Issue #2703063: Refactor base classes for extensibility #35
Conversation
| * @code parent::__construct() @endcode. | ||
| */ | ||
| public function __construct() { | ||
| $rules = module_invoke_all('fb_instant_articles_transformer_rules'); |
There was a problem hiding this comment.
This makes total sense to me. So then the fb_instant_articles_display module would implement this hook and add a default set of rules, which are now in transformer_config.json.
There was a problem hiding this comment.
Makes sense to me as well. I really like the extensibility of this approach 👍
@m4olivei How do you wan to go about implementing in fb_instant_articles_display? I'm thinking it needs to be a new PR stacked on top of this one (since this currently removes the loading of rules from json, effectively breaking the functionality of fb_instant_articles_display), right?
There was a problem hiding this comment.
Hey @simonengelhardt yep. i wanted to talk through the concept of these base classes, and how it would affect the existing display class before i wrote that part. I'm planning to refactor the rules using this hook in a few mins. I did want to reach out to you about a possible addition to the SDK though, to help with this… I'll ping you on the fb chat about it
There was a problem hiding this comment.
Hum, been testing and something still isn't right. If I implement the hook that is fired here: https://github.com/scottrigby/module-fb_instant_articles/blob/2703063_refactor_base_classes_for_extensibility/src/InstantArticle.php#L27, the $this parameter that is passed there is not populated with all pieces of the Instant Article that should have been added via the field formatters.
The reason is, we effectively have two instances of \Facebook\InstantArticles\Elements\InstantArticle floating around. The first is a \Facebook\InstantArticles\Elements\InstantArticle proper and the second is a Drupal\fb_instant_articles_display\InstantArticle. The \Facebook\InstantArticles\Elements\InstantArticle class is created here: https://github.com/scottrigby/module-fb_instant_articles/blob/2703063_refactor_base_classes_for_extensibility/modules/fb_instant_articles_display/src/InstantArticle.php#L83. The second, is created here, and the first is stored as a member of the second: https://github.com/scottrigby/module-fb_instant_articles/blob/2703063_refactor_base_classes_for_extensibility/modules/fb_instant_articles_display/src/InstantArticle.php#L115.
The field formatters then act on the \Facebook\InstantArticles\Elements\InstantArticle object instance, eg. https://github.com/scottrigby/module-fb_instant_articles/blob/2703063_refactor_base_classes_for_extensibility/modules/fb_instant_articles_display/src/InstantArticle.php#L138,L139. But then the alter hook passes the Drupal\fb_instant_articles_display\InstantArticle object instance: https://github.com/scottrigby/module-fb_instant_articles/blob/2703063_refactor_base_classes_for_extensibility/src/InstantArticle.php#L27.
In the end what you get is empty articles in your output :/.
Hopefully that makes sense, it's confusing to keep straight b/c there are three classes named InstantArticle all under different namespaces.
Trying to think about a resolution.
There was a problem hiding this comment.
Not sure we can get to where we want to be here given the constraints of the SDK. I'm thinking one solution could be that Drupal\fb_instant_articles\InstantArticle simply stands alone, not extending anything but soaks up what might be common to Drupal\fb_instant_articles_display\InstantArticle and any other such child class, but otherwise stays the same to what it is in 7.x-1.x.
And/or we move to a decorator pattern, which would implement all the public methods of Drupal\fb_instant_articles\InstantArticle and simply delegate to the object instance which is a member of the class for those methods, except for when we want to do something special, eg. for render().
Although it's starting to feel like a lot of effort for something maybe not as crucial as we imagine it to be?
There was a problem hiding this comment.
I agree with the pattern problem. The single reason for doing this is to allow other modules the ability to alter the SDK InstantArticle object without requiring the Display module. I'm opening another PR with an alternate approach.
… the InstantArticle object can check context if they expect a Drupal entity
|
Closing this PR in favor of #55. |
This isn't fully fleshed out, but opening a PR now for discussion as we count down to RC1 by tonight. @m4olivei this is what we discussed, allowing other modules (for example a custom ad module) to provide custom rules, and transform the instant article HTML, in a way that can be used with or without the Display module. Let's look at talk about it… and then we can modify as needed, then press ahead with finishing refactoring the display class etc.