BUG Manually create singleton when building table #9952

maxime-rainville · 2021-05-25T23:08:30Z

Because we use singleton to get class references when building table, if you try to inject over a class, it's table will never be created.

Parent issue

Overriding classes with Injector breaks MTI #9950

maxime-rainville · 2021-05-25T23:09:18Z

This may be a naive fix, but at first glance looks like it's working fine. Not quite sure if this is something that is testable.

michalkleiner · 2021-05-26T02:18:40Z

I'll give it a crack. I think it is testable.

maxime-rainville · 2021-05-26T02:23:33Z

Just clarifying: I meant "testable" as is "can write unit test to cover this use case". I tested it locally with a similar example as the one provided by @dhensby.

michalkleiner · 2021-05-26T02:56:28Z

Yep, I got what you meant. That's why I wrote two independent sentences :)

I think it's testable as long as we can assert what db tables get built and what columns they contain.

dhensby · 2021-05-26T08:44:58Z

a slight variation on #9951 (which I'll close).

I had targeted 4.2 as it is potentially the lowest supported version of the framework, given it's a fairly high impact bug it deserves to be back-ported as far as reasonable.

GuySartorelli · 2022-05-13T01:33:01Z

I don't think we can add unit tests for this... When the database is built for tests it doesn't use this code at all - it uses SilverStripe\ORM\Connect\TempDatabase::rebuildTables() which also uses the singleton function.

So any test cases written to test this functionality right now would fail with this PR because the method used for creating test database tables is using singleton() (which I think we should address in this PR so that the way test DB tables are made mirrors the way production ones are made) - but even addressing that, they won't test that the actual production functionality works.

silverstripe-framework/src/ORM/Connect/TempDatabase.php

Line 252 in 995e5dc

$SNG = singleton($dataClass);

silverstripe-framework/src/ORM/Connect/TempDatabase.php

Line 262 in 995e5dc

$SNG = singleton($dataClass);

GuySartorelli · 2022-07-05T23:47:01Z

I've found what I think is a pretty clean solution to be able to test this - I've created a new class to hold the logic that is shared between DatabaseAdmin and TempDatabase as far as building tables is concerned. This way we can write a test for that logic which will be valid, because it's the same logic shared between tests and prod db.

GuySartorelli · 2022-07-06T00:18:02Z

Scrutinizer failure is unrelated to this PR

michalkleiner · 2022-07-06T15:29:16Z

src/ORM/Connect/TableBuilder.php

+                // If we have additional dataobjects which need schema (i.e. for tests), do so here:
+                if ($extraDataObjects) {
+                    foreach ($extraDataObjects as $dataClass) {
+                        $SNG = new $dataClass([], DataObject::CREATE_SINGLETON);
+                        if ($SNG instanceof DataObject) {
+                            $SNG->requireTable();
+                        }
+                    }
+                }
+            }


This runs for each input dataClass whereas previously it was outside of the loop. Should this be moved one level up after the first foreach?

Ahh, yes - my bad. I'll fix that.

michalkleiner · 2022-07-06T15:31:08Z

Nice work @GuySartorelli!
I didn't get to test this yet as it's on a project we parked for a bit and couldn't spend too much time on, but I'll be able to confirm this works at some point in future.

GuySartorelli · 2022-07-06T21:50:00Z

Probably also worth me mentioning that I have confirmed the tests I've added fail before the changes in this PR.

michalkleiner · 2022-07-06T22:17:40Z

Cool, thanks for hopping in and pushing it over the line.
Do we need want to retarget at minor since it's fixing a bug? Or do we see it as a big enough change in the mechanism that it should be seen as an enhancement? I don't think we know when this stopped working.

GuySartorelli · 2022-07-06T22:41:17Z

I targetted 4 specifically because there is increased API surface with this approach - technically this is an API PR even though it's to fix a bug.
But it's also effectively internal API (even though it's a new class with a public method) so I could see an argument for it being a patch release.

michalkleiner · 2022-07-07T09:41:03Z

I would probably lean towards a patch fix due to the nature of the issue, but it can really be argued for both ways. But since we made several workarounds, if I didn't know about this change and just simply applied a new patch release, the site would stop working since the data would become inconsistent in different tables. But also I'd prefer fixing that and having working MTI — 🤷

GuySartorelli · 2022-07-07T23:45:55Z

I'll defer the branch targetting conundrum to whoever wants to merge this - happy to retarget and fix commits to match whatever branch the merger chooses.

michalkleiner · 2022-07-11T19:53:00Z

@maxime-rainville wanna decide this one for us? :-D I'm happy to merge it as a fix with your blessing.

sabina-talipova · 2022-07-12T03:18:27Z

Test in local environment. All works.
Approve and merge.

michalkleiner · 2022-07-12T14:31:26Z

Thanks for jumping in for the merge, @sabina-talipova, though I sort of wanted to know what Max's opinion was.

Since we've now merged this into 4, without @maxime-rainville giving his view, the question now is — should we backport this into 4.11?

maxime-rainville · 2022-07-12T21:50:16Z

It is adding a brand new Class to our API, so it should ship in a minor release.

If this was a major bug and people had been screaming for a fix forever, I'd say maybe it's worth shipping this in a patch release, but as far as I can tell it's always been broken and it took us until last year to notice.

It's a smallish change but it affects some pretty low level methods. I would rather take it through our regular minor release regression test.

maxime-rainville changed the base branch from 4 to 4.7 May 25, 2021 23:08

maxime-rainville mentioned this pull request May 25, 2021

Overriding classes with Injector breaks MTI #9950

Closed

2 tasks

dhensby mentioned this pull request May 26, 2021

FIX Build classes even if they have been overridden #9951

Closed

GuySartorelli changed the base branch from 4.7 to 4.10 May 13, 2022 01:53

GuySartorelli mentioned this pull request Jul 5, 2022

Add extension hooks to update submitted form fields before processing silverstripe/silverstripe-userforms#1060

Closed

GuySartorelli force-pushed the pulls/4.7/allow-class-to-inject-over-parent branch from cdff042 to 6a33725 Compare July 5, 2022 21:54

GuySartorelli changed the base branch from 4.10 to 4.11 July 5, 2022 21:54

GuySartorelli changed the base branch from 4.11 to 4 July 5, 2022 23:27

BUG Manually create singleton when building table

33600c3

GuySartorelli force-pushed the pulls/4.7/allow-class-to-inject-over-parent branch from 6a33725 to 1140cb3 Compare July 5, 2022 23:30

michalkleiner reviewed Jul 6, 2022

View reviewed changes

GuySartorelli added 2 commits July 7, 2022 09:49

API TableBuilder class reduces inconsistency between temp & prod db

29fae72

MNT Add tests for overriding DataObject via injection

0482444

GuySartorelli force-pushed the pulls/4.7/allow-class-to-inject-over-parent branch from 7017431 to 0482444 Compare July 6, 2022 21:49

sabina-talipova approved these changes Jul 12, 2022

View reviewed changes

sabina-talipova merged commit 2262d84 into silverstripe:4 Jul 12, 2022

sabina-talipova deleted the pulls/4.7/allow-class-to-inject-over-parent branch July 12, 2022 03:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG Manually create singleton when building table #9952

BUG Manually create singleton when building table #9952

maxime-rainville commented May 25, 2021

maxime-rainville commented May 25, 2021 •

edited

Loading

michalkleiner commented May 26, 2021

maxime-rainville commented May 26, 2021 •

edited

Loading

michalkleiner commented May 26, 2021

dhensby commented May 26, 2021

GuySartorelli commented May 13, 2022 •

edited

Loading

GuySartorelli commented Jul 5, 2022

GuySartorelli commented Jul 6, 2022

michalkleiner Jul 6, 2022

GuySartorelli Jul 6, 2022

michalkleiner commented Jul 6, 2022

GuySartorelli commented Jul 6, 2022

michalkleiner commented Jul 6, 2022

GuySartorelli commented Jul 6, 2022

michalkleiner commented Jul 7, 2022

GuySartorelli commented Jul 7, 2022

michalkleiner commented Jul 11, 2022

sabina-talipova commented Jul 12, 2022

michalkleiner commented Jul 12, 2022

maxime-rainville commented Jul 12, 2022

BUG Manually create singleton when building table #9952

BUG Manually create singleton when building table #9952

Conversation

maxime-rainville commented May 25, 2021

Parent issue

maxime-rainville commented May 25, 2021 • edited Loading

michalkleiner commented May 26, 2021

maxime-rainville commented May 26, 2021 • edited Loading

michalkleiner commented May 26, 2021

dhensby commented May 26, 2021

GuySartorelli commented May 13, 2022 • edited Loading

GuySartorelli commented Jul 5, 2022

GuySartorelli commented Jul 6, 2022

michalkleiner Jul 6, 2022

Choose a reason for hiding this comment

GuySartorelli Jul 6, 2022

Choose a reason for hiding this comment

michalkleiner commented Jul 6, 2022

GuySartorelli commented Jul 6, 2022

michalkleiner commented Jul 6, 2022

GuySartorelli commented Jul 6, 2022

michalkleiner commented Jul 7, 2022

GuySartorelli commented Jul 7, 2022

michalkleiner commented Jul 11, 2022

sabina-talipova commented Jul 12, 2022

michalkleiner commented Jul 12, 2022

maxime-rainville commented Jul 12, 2022

maxime-rainville commented May 25, 2021 •

edited

Loading

maxime-rainville commented May 26, 2021 •

edited

Loading

GuySartorelli commented May 13, 2022 •

edited

Loading