Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support late static binding across all classes #232

Merged
merged 37 commits into from
Oct 22, 2019
Merged

Support late static binding across all classes #232

merged 37 commits into from
Oct 22, 2019

Conversation

schlessera
Copy link
Contributor

The classes have a mixture of practices right now which make them very tricky to extend.

All classes seem to communicate they can be extended (i.e. they are not marked as final), but as most of them don't properly support late static binding (they use self:: and private methods), extending them will lead to unexpected results and hard-to-diagnose bugs. self:: and private should only be used in final classes, or, in more rare cases, when an extension should specifically not have access to some part sof the base class.

The solution is to either mark all of them as final (provided there is an underlying interface), or make sure they properly support extension.

I've opted for the second option in this PR, for the following reasons:

  • some classes were already changed from non-extensible to extensible in prior changes, so I suppose this is a wanted use case
  • some classes have quite a bit of logic to them, and keeping them extensible avoids completely rewriting some of these

Related issue: #231

@schlessera
Use late static binding with BaseTranslator::$current
e71df7e 
The class is an abstract base class and the `$current` is `public static`.

Accessing it via `static::` instead of `self::` makes sure an extending class won't inadvertently create an alternate dynamic variable `$current` if it overrides `BaseTranslator::register()`.
@schlessera
Use late static binding in Translator class
26a78e2 
Accessing its methods via `static::` instead of `self::` makes sure that method overrides in extending classes will be respected.

Also makes its properties protected instead of private, to avoid dynamic redeclaration in extending classes.
@schlessera
Make CsvTrait handle class extensions gracefully
3b2af79 
Use `static::` instead of `self::` and make the private methods protected instead.

This avoids the trait breaking if it is used in a base class that will be extended, and one of the trait methods is extended as well.

This is a safer default for traits in general.
@schlessera
Make DictionaryTrait handle class extensions gracefully
7895031 
Use `static::` instead of `self::` and make the private methods protected instead.

This avoids the trait breaking if it is used in a base class that will be extended, and one of the trait methods is extended as well.

This is a safer default for traits in general.
@schlessera
Make HeadersExtractorTrait handle class extensions gracefully
4617389 
Use `static::` instead of `self::` and make the private methods protected instead.

This avoids the trait breaking if it is used in a base class that will be extended, and one of the trait methods is extended as well.

This is a safer default for traits in general.
@schlessera
Make HeadersGeneratorTrait handle class extensions gracefully
f1b76aa 
Make the private methods protected instead.

This avoids the trait breaking if it is used in a base class that will be extended, and one of the trait methods is extended as well.

This is a safer default for traits in general.
@schlessera
Make MultidimensionalArrayTrait handle class extensions gracefully
99e6f1e 
Use `static::` instead of `self::` and make the private methods protected instead.

This avoids the trait breaking if it is used in a base class that will be extended, and one of the trait methods is extended as well.

This is a safer default for traits in general.
@schlessera
Turn private method in FunctionsScanner abstract class into protected
0d9c665 
Having a non-final class with private methods is problematic. In this case, overriding `saveGettextFunctions()` would make `deconstructArgs()` inaccessible and break.
@schlessera
Use late static binding in JsFunctionsScanner class
f3c9546 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in ParsedComment
49ff670 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\Csv
b940bd4 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\CsvDictionary
f0b8d8e 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\Jed
a0c0ce2 
Also turn `private` method into `protected` method.

This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Json
2696458 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\JsonDictionary
98d61c2 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\Mo
71e1046 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\PhpArray
d2925e5 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in ParsedComment
399b4e8 
Also turns `private` methods into `protected`.

This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\Xliff
732e942 
Also turns `private` methods into `protected`.

This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\Yaml
baeb72c 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Generators\YamlDictionary
27ace45 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\Csv
4e54e4e 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\CsvDictionary
1e27944 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\Extractor
f3e6f15 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\Jed
5e8cbef 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\JsCode
b0aee0e 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\Json
0853e6e 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\JsonDictionary
a4c4083 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\Mo
2bbfc43 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\PhpArray
eecac45 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\PhpCode
117f061 
Also turns `private` methods into `protected`.

This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\Po
e3e93e2 
Also turns a `private` method into `protected`.

This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\Twig
e3d9da7 
Also turns a `private` method into `protected`.

This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\VueJs
397e4bd 
Also turns `private` methods into `protected`.

This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\Yaml
3f143d6 
This avoids extending classes unexpectedly breaking.
@schlessera
Use late static binding in Extractors\YamlDictionary
0fe3448 
This avoids extending classes unexpectedly breaking.
@schlessera
Merge branch 'master' into replace-self-with-static-in-extendable-cla…
6a6d98e 
…sses
@oscarotero oscarotero merged commit c51ac33 into php-gettext:master Oct 22, 2019
@schlessera schlessera mentioned this pull request Apr 9, 2020
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@schlessera @oscarotero