Skip to content

Convert all private members on Assetic\Asset\* classes to protected. #348

Closed
wants to merge 1 commit into from

3 participants

@sdboyer
sdboyer commented Jan 3, 2013

i'm making this request as part of the overall attempt to shift Drupal to using Assetic. see https://drupal.org/node/1871596, but especially https://drupal.org/node/1871596#comment-6884638 .

we have to subclass Assetic's assets classes in order to accommodate some additional metadata layers (we have to deal with asset reordering in a way that Assetic does not appear to be built for). so it's really quite important that we be able to mutate, or at least access, these various properties. BaseAsset provides getters for most of the properties, but HttpAsset, for example, provides no getter for $sourceUrl. even with getters for everything, though, the code would be awkward and verbose to do all such assignments/accessings via methods.

the alternative is simply writing our own BaseAsset class, which would be pretty awful - i'd like us to keep things DRY.

i certainly understand not wanting the properties public, but i don't see any obvious reason to not make them protected. i haven't spent a TON of time looking at the AssetInterface family tree to figure out any implicit contracts there, but nothing has jumped out at me so far that really necessitates restricting access to those members by subclasses.

i realize there's a somewhat similar issue over at #236, but that deals with members on the writer classes, on which @schmittjoh's comments about the use of composition seem more applicable. in this case, i'd prefer to avoid composition because it would make serialization (something else we'll likely need to do) more complex, make the class structure itself more complex in general, and all without a use case for utilizing the swappability composition brings.

@sdboyer
sdboyer commented Jan 4, 2013

i'm sure we could also work find a solution where we convert only some subset of the members to protected - it's just easier for me to bundle all the changes together into a single PR than to make requests individually as i find myself needing to override/access them.

@stof
Collaborator
stof commented Jan 4, 2013

The issue when converting everything to protected is that they become available as extension point, meaning we also have to consider the BC on all the properties. This is the reason why we use private by default (as done in Symfony)

@sdboyer
sdboyer commented Jan 4, 2013

ok sure, private certainly insulates you from that. but that doesn't address the lack of getters/setters for certain properties.

i get private as a default policy, but your response doesn't give any indication as to whether or not you're willing to allow extension points (and introduce the BC consideration) in this particular case.

@sdboyer
sdboyer commented Jan 4, 2013

for the moment, i'm proceeding with my own, independent BaseAsset at the root of my class hierarchy.

@kriswallsmith
Owner

Would adding getters and setters address your issue?

@sdboyer
@kriswallsmith
Owner

I'm going to close this, but please reopen or open a new ticket any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.