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.

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

Correct private static references #74

Closed
sminnee opened this issue May 13, 2018 · 10 comments

Comments

Projects
None yet
7 participants
@sminnee
Copy link
Member

commented May 13, 2018

Overview

A common thing I've found in projects I've been upgrading is that $db etc are referred to as public statics. Although unorthodox in SS3, it silently worked. In SS4 it causes warnings.

It should be feasible to rewrite a common whitelist of static properties to private statics. Those from DataObjects, Controllers, and ModelAdmins seem the most common.

Acceptance Criteria

  • Devs can opt into this through a specific upgrader task (or flag)
  • Rewrites ALL private statics (to keep implementation simple)
  • Approach is well documented in upgrading guide

Notes

  • Bonus points: Limit to implementors of Configurable (incl. subclasses of those implementors). Timeboxed to 4h

Out of Scope

  • Opt-out per declaration via @ internal
  • Opt-in per declaration via @ config

Pull Requests

@chillu

This comment has been minimized.

Copy link
Member

commented Jul 8, 2018

I've confirmed that those warnings surface on dev/build, at least for the . Hence I'm downgrading this from impact/high to impact/medium, because there's a pretty obvious way to fix it after running the upgrader.

image.png

@chillu chillu added impact/medium and removed impact/high labels Jul 8, 2018

@sminnee

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2018

Okay, but this is a pretty tedious part of upgrades... can it be “extra medium”? ;-)

@chillu

This comment has been minimized.

Copy link
Member

commented Jul 8, 2018

We've been recommending private static for many years, I don't think this is widespread?

@sminnee

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

Maybe I drew the short straw, but it was present on each of the 2 or 3 projects the I found.

If anyone else has seen common use of non-private statics in their sites, maybe post here? Otherwise if I was the unlucky one this can remain lower priority.

@robbieaverill

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

I've come across public statics a few times, or statics without a visibility (public by default). I think I'm with @chillu in that it's less common now though and there's a clear way to see and fix them already.

Unless they're tagged with @config PHPDocs it'd be a little dangerous to auto-rewrite public statics to private statics.

@maxime-rainville

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

Part of what makes this one difficult is that we don't have a nice elegant way of saying this this attribute/method that use to be public should now be private. A similar constraint is in play in #18.

If we had a way to define visibility updates in .upgrade.yml this would be easy to fix.

@dhensby

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

I'd say that it's a pretty safe assumption that public statics in SS code should be private static now. I think that'd be right about 90% of the time. It's a lot easier to provide this functionality, and for a dev to then use git add -p to only accept the changes that were right and discard the others than it is to go through and manually upgrade all of them.

@TomasVotruba

This comment has been minimized.

Copy link

commented Jul 29, 2018

I'm starting to looking into feature that could Rector handle based #128

Do you any code snippet for this feature showing the before/after?

-$someService::$staticProperty;
+$someService::$privateProperty;
@dhensby

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

It's basically:

class Page extends SiteTree {
-    public static $db = array(
+    private static $db = array(
        ...
    );
}
@maxime-rainville

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

Everything's been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.