-
Notifications
You must be signed in to change notification settings - Fork 382
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
Private property in extensible class AMP_Base_Sanitizer
#3061
Labels
Comments
Good point. Yes, the method should be marked As part of #2315 it may make sense for this to be |
By the way, do you have a problem with my command of the English language‽ 025d1bb 😉 |
@schlessera @westonruter it seems this is an easy fix? Can we go ahead with it? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
The extensible class
AMP_Base_Sanitizer
has a private property$should_not_removed_nodes
[sic].It is being used in a public method
remove_invalid_child()
, and if an extending class would override this public method, for example by copying the source code over and making modifications, this might not behave as expected. Copying the code as is from the base method would create a separate, dynamic property with a lifetime independent of the original one, and the code would act either on one or the other, depending on what class it comes from.Is the use of
private
intentional here? If yes, would it make sense to make theremove_invalid_child()
final to make sure no one overrides it, introducing hard-to-diagnose bugs?If not, it might make sense to turn the property from
private
toprotected
, so it behaves more consistently with regards to inheritance.The text was updated successfully, but these errors were encountered: