Skip to content

Use DBClassName so that dev/build can migrate legacy classnames - #852

Open
tractorcow wants to merge 1 commit into
silverstripe:4from
tractorcow:pulls/4/classname
Open

tractorcow wants to merge 1 commit into
silverstripe:4from
tractorcow:pulls/4/classname

Conversation

@tractorcow

Copy link
Copy Markdown
Contributor

I had a few issues where I'd register a class name map with DatabaseAdmin, but elemental wasn't picking this up.

Slashes are double escaped intentionally, in case it looks weird. :P

{
private static $db = [
'OwnerClassName' => 'Varchar(255)',
'OwnerClassName' => 'DBClassName("SilverStripe\\\\ORM\\\\DataObject")',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be DBClassName("SilverStripe\\\\ORM\\\\FieldType\\\\DBVarchar") to keep it effectively a Varchar? Don't understand the change to DataObject.

@tractorcow tractorcow Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the base class that DBClassName should use. The Value of 'OwnerClassName' is a subclass of dataobject.

If you don't add a base name it will assume self::class (which is ElementalArea, and thus wrong).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalkleiner I've just skim read this, but the change from varchar to DBClassName is to make it a proper polymorphic relationship, which means that dev/build in framework can perform modifications on it like it does for other polymorphic relations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should build a proper polymorphic belongs_to in core right @robbieaverill ? :P

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! I guess it this were replaced with Owner -> DataObject then it'd be even more real

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants