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

Don't upgrade classnames in array key strings #135

Closed
chillu opened this issue Oct 2, 2018 · 6 comments

Comments

Projects
None yet
6 participants
@chillu
Copy link
Member

commented Oct 2, 2018

Acceptance Criteria

  • As a developer upgrading a 3.x site, I have good visibility of when the upgrader performs ambiguous actions
  • Upgrader shows explicit diff on string replacements to clearly point out the ambiguity to the developer

Details

Before:

$has_one = array('Image' => 'Image')

After:

$has_one = array(Image::class => Image::class)

Expected:

$has_one = array('Image' => Image::class)

Related: #16

/cc @pitchandtone

Pull Requests

@pitchandtone

This comment has been minimized.

Copy link

commented Oct 2, 2018

There's examples in code where you do things like new UploadField('Image') which then becomes new UploadField(Image::class) which is also not helpful. It might not be the only class to have this issue (maybe Member too) but has been a pain to tidy up after the script.

@Leapfrognz

This comment has been minimized.

Copy link

commented Oct 2, 2018

Another example:
UploadField:create('Image', 'Image') becomes UploadField:create('SilverStripe\Assets\Image', 'SilverStripe\Assets\Image')
UploadField:create('Link', 'Link') becomes UploadField:create('Sheadawson\Linkable\Model\Link', 'Sheadawson\Linkable\Model\Link')

@chillu

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

Yeah, it gets pretty tricky. We could whitelist specific methods and properties which are known to accept object types (in specific positions), but that's a big list. PHP code introspection is hard.

@maxime-rainville maxime-rainville self-assigned this Oct 28, 2018

@unclecheese

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2018

Also EmailField::create('Email')..

We should identify SS3 classes with common names, like Image and Email, and maybe only change those strings on user confrirmation.

@chillu

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2018

There's too many edge cases here, the best we can do is to highlight this more explicitly to devs. I've reflected this in the ACs

@maxime-rainville

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

silverstripe/silverstripe-framework#8741 has not been merged yet.

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.