Hi,

Thanks for all the work on Feeds. I think there might be an issue in the code, but I'm not sure if the comment's off, or the code (or me!).

FeedsConfigurable::__get() is documented with Override magic method __get(). Make sure that $this->config goes through getConfig(). The implementation does a bit more than that though:
it publicly exposes any private members that FeedsConfigurable itself may have (none at the mo), and also any protected members declared anywhere in the inheritance hierarchy. Also it doesn't generate a warning for non-existent members.

If you're happy with the implementation as it is, I think it would be nice to expand the comment to explain its effect.

Thanks again

CommentFileSizeAuthor
#8 feeds-feedsconfigurable-__get-comment-2636342-8.patch1.2 KBmegachriz
#4 feeds-feedsconfigurable___get_comment-2636342-4-D7.patch1.11 KBandyf

Comments

AndyF created an issue. See original summary.

andyf’s picture

andyf commented
Title: FeedsConfigurable::__get exposes desdcendents' protected members » FeedsConfigurable::__get exposes descendents' protected members
megachriz’s picture

megachriz
Primary language Dutch
commented
Priority: Normal » Minor

It seemed to be added on purpose to "fix warnings". See commit.

Anyway, I'm fine with the implementation as it is. Code in the module and probably also contrib code already tries to access private/protected members. As long as these properties can't be set outside of the class, I don't see any concerns.

It's a good idea to expand the documentation of this method. Any suggestion for the additional documentation?

andyf’s picture

andyf commented
StatusFileSize
new feeds-feedsconfigurable___get_comment-2636342-4-D7.patch1.11 KB

OK so I think maybe there's another difference I hadn't noticed. IIUC the magic function won't be used for $this->config as written in the comment. (__get() is utilized for reading data from inaccessible properties.) I don't know if other code is relying on that anywhere?

Anyways, I've attached a patch with a suggestion for updating the comment.

Thanks

andyf’s picture

andyf commented
Status: Active » Needs review
megachriz’s picture

megachriz
Primary language Dutch
commented

You are right about $this->config. Within the class itself and in child classes the method __get() isn't called when accessing $this->config. I looked at the code of child classes, but could not see anything that might cause something bad so far. Note that in the construct method of FeedsConfigurable the result of FeedsConfigurable::configDefaults() is assigned to $this->config. This method is also called in FeedsConfigurable::getConfig().

  1. +++ b/includes/FeedsConfigurable.inc
@@ -15,6 +15,9 @@ class FeedsNotExistingException extends Exception {
+ * Due to the magic method __get(), descendents' protected properties will be
+ * publicly accessible (but not writeable).

    I think I would change "publicly accessible" to "publicly readable". 

  2. +++ b/includes/FeedsConfigurable.inc
@@ -139,8 +142,12 @@ abstract class FeedsConfigurable {
+   * - expose our own private and protected properties, and descendents'
+   *   protected properties;

    Maybe it is enough to document that own private and protected properties are exposed and remove the note about descendents properties. Classes that extend FeedsConfigurable could in theory override __get(). And FeedsConfigurable doesn't know by itself that it is extended (we know though). What do you think?

andyf’s picture

andyf commented

I think I would change "publicly accessible" to "publicly readable".

Thanks, I'll do that.

Classes that extend FeedsConfigurable could in theory override __get(). And FeedsConfigurable doesn't know by itself that it is extended (we know though).

Personally, I'd argue that the comment is only describing the behavior of the method, which is that it exposes descendent protected properties. If we later derive from that class and override __get() and change the behaviour, it should then be documented in the derived class __get().

Just my 2 cents, thanks!

megachriz’s picture

megachriz
Primary language Dutch
commented
Component: Code » Documentation
StatusFileSize
new feeds-feedsconfigurable-__get-comment-2636342-8.patch1.2 KB

What do you think of the documentation in the attached patch?

andyf’s picture

andyf commented

+1 seems easier to understand to me, thanks!

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

Thanks for your feedback, AndyF! Committed #8.

  • MegaChriz committed 46d1119 on 7.x-2.x 
    Issue #2636342 by AndyF, MegaChriz: Improved documentation of...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.