Support fluent and clean up #32

adrhumphreys · 2020-08-07T00:56:11Z

This allows you to now support using the module with fluent
It also splits out objects into classes which we can find the tables for and then tables which are just truncated
Task is now run from populate-task and documented correctly for running it

jakxnz

Looking good. I was actually just picking this branch up for a project and thought I'd give it a quick review.

Also, no PR emoji @adrhumphreys ? 😎

jakxnz · 2020-09-04T02:56:48Z

code/Populate.php

+    }
+
+    /*
+     * Attempts to truncate a table if it hasn't already been truncated


Would it be worth completing the docBlock?

jakxnz · 2020-09-04T03:01:16Z

code/Populate.php

+            }
+        }
+
+        if ($obj->hasExtension(FluentExtension::class)) {


Instead of baking this directly into the logic, could you create an extension point here, and use the config to apply an extension for fluent, when fluent is in the project?

i.e

Only: moduleexists: 'Fluent'

While it's a complete breakdown if fluent is installed, it still feels quite opinionated to include this concern here.

I like this idea better in general, but we already have hooks in here for checking whether the object is Versioned or not (Versioned is part of core but it's still optional on objects).

Let's merge this in as-is, and then I'll create an issue to suggest this change.

adrhumphreys · 2020-09-05T03:55:44Z

@jakxnz I've become old and can't relate to emojis anymore 😂

You might want to try out https://github.com/adrhumphreys/silverstripe-fixtures

I found that with Fluent, Elemental, and Populate, you end up spending more time debugging missing data in some key table or some such. I still need to get around to updating this PR to reflect the changes I made when making the module

jakxnz · 2020-09-06T21:42:22Z

README.md

+Example:
+```yaml
+DNADesign\Populate\Populate:
+  truncate_classes:


Is it worth adding an "upgrade" note, for users who don't notice the change in terminology here?

jakxnz · 2020-09-06T21:54:29Z

code/Populate.php

+    private static function deleteClass(string $class): void
+    {
+        // First delete the base classes
+        $tableClasses = ClassInfo::ancestry($class, true);


This used to also capture downstream tables as well, e.g ClassInfo::dataClassesFor(), so only capturing ancestry is quite a deviation. What's the advantage of the change?

jakxnz · 2020-09-07T03:01:49Z

I did end up testing this branch, and found that it didn't quite manage a clean slate, and so my populate task crashed due to duplicate keys.

You might want to try out https://github.com/adrhumphreys/silverstripe-fixtures

Thanks for the suggestion. Can you elaborate on what that solves @adrhumphreys ?

adrhumphreys · 2020-09-11T00:34:03Z

@jakxnz just need to find time to update the autocomplete code a bit 🙏🏽
The module solves the issue of needing to have a lot of specificity in the order things need to be loaded and increasing the ease of adding in functionality to that creation. I also found often you'll not have put a correct value in, say you have a default value set on write, that doesn't get automatically set anymore so you need to specify it

You end up with these hard to read yml files that sort of go like:

create the elemental area for the page
create the elemental area for the layout block
create the page and link to the area
create the carousel block and link to the area

Where it becomes more difficult to conceptualise what is being done in the populate config

The idea with the other module is you create fixtures in PHP in which you call the normal creation methods that you have in Silverstripe. If you want to create 50 objects with random numbers you can just write a loop to do that.
It manages the loading order by allowing the developer to explicitly state the dependencies for a fixture rather than manually ordering them
It also provides a few purging options, you can choose any/all of the three, array of classes to delete, array of tables to delete, a method called unload which you can write your own code in

This isn't all smooth though as you'll have a performance hit since you're calling all the after writes etc for your models now when populating them. I just found the tradeoff worth it

jakxnz · 2020-09-11T01:51:55Z

The module solves the issue of needing to have a lot of specificity in the order things need to be loaded and increasing the ease of adding in functionality to that creation

Nice. Yep, I have experienced this. Cool that you're working on an enhancement! @adrhumphreys

For anyone looking for a way to support Fluent via populate, I raised a PR against @adrhumphreys fork, with some adjustments that worked for our project adrhumphreys#1

Truncate tables extension hook

README.md

michalkleiner · 2020-09-14T05:01:56Z

code/Populate.php

-            self::truncateTable($table);
+        $tables = [];
+
+        // All acenstors or children with tables


typo in 'ancestors'

code/Populate.php

madmatt · 2021-10-11T22:50:46Z

Hey @adrhumphreys there's a few outstanding changes suggested on this, do you still want to look at merging it in at some point?

I'm just trying to work out which order to merge changes in - whether merging #26, #32 then #33 makes the most sense or something else...

adrhumphreys · 2021-10-12T19:54:35Z

Hey @madmatt I haven't actually used the module since I made these changes. I do know there's some internal project(s) using the changes as a fork though so I imagine it's probably higher on the list of wanting to merge in

jakxnz · 2021-10-12T20:16:32Z

@madmatt the suggestions are grammar changes and a disputed suggestion about a method name. Are they a trivial reason to leave a PR on hiatus?

The track record demonstrates that the people who progressed the contribution this far haven't had the capacity to address the outstanding nitpicks. As far as I understand it, there's greater internal benefit to having this merged than leaving it as a liability when support is needed for projects that implement it. Is there value enough there for you to get this merged @madmatt ? Or do we need to wait until the contributors of this PR (@adrhumphreys or I) have capacity?

madmatt · 2021-10-13T20:56:01Z

I've fixed all the conflicts in README.md and rebased/merged the master branch in.

Any final thoughts @jakxnz, @adrhumphreys, @michalkleiner or @chrispenny?

If not, I'm happy to merge it now and we can tidy things up later.

One question I have is how does it actually support fluent now @adrhumphreys? I see there is some old code that supported it by checking if the Fluent extension was applied to an object that was being truncated, but this appears to have been removed. I don't see anything added to the fluent module either, so does it just handle it implicitly now and you didn't have to do anything? Or am I missing something obvious here? 🙂

madmatt · 2021-10-13T21:16:18Z

FWIW, I've made some tweaks to README:

Re-instate the warning about not installing in production and make it much clearer. While there is a check in the code to ensure it's only running on dev or test environments, that's not enough (you could fool it with isDev=1, and I've seen many production sites running in dev mode accidentally). So best to reinforce the 'install with --dev' message here.
Adjust messaging around Versioned and Fluent to be clearer - on the assumption that this does support Fluent (see my previous comment as I can't understand how that works).

michalkleiner · 2021-10-13T21:35:38Z

All good with me, all the remaining stuff is quite minor.

jakxnz · 2021-10-13T22:05:29Z

@madmatt , it currently "supports" fluent by exposing an extension point that can be used to clear tables created by other modules e.g

PopulateExtension.php:

/**
 * @param array $tables
 * @param string $className
 * @param array $classTables e.g ['Class\Name' => 'Table']
 */
public function updateTruncateObjectTables(&$tables, &$className, &$classTables)
{
    // Update truncate object tables
    foreach ($classTables as $className => $baseTable) {
        /** @var DataObject|Versioned|FluentExtension $obj */
        $obj = Injector::inst()->get($className);

        if (!$obj->hasExtension(FluentExtension::class)) {
            // No localised tables to clear
            continue;
        }

        $fluentTable = $obj->getLocalisedTable($baseTable);

        $tables[$fluentTable] = $fluentTable;

        if (!$obj->hasExtension(Versioned::class)) {
            // No localised versioned tables to clear
            continue;
        }

        $stages = $obj->getVersionedStages();

        foreach ($stages as $stage) {
            $table = $obj->stageTable($fluentTable, $stage);

            // Include localised base and live table
            $tables[$table] = $table;
        }

        $versionedTable = "{$fluentTable}_Versions";

        // Include localised versions table
        $tables[$versionedTable] = $versionedTable;
    }
}

I felt this approach was less opinionated and had better grouping of concerns.

The title of the PR is a bit misleading.

chrispenny · 2021-10-20T18:21:16Z

code/PopulateTask.php

+    /**
+     * @var string
+     */
+    private static $segment = 'populate-task';


Just as a heads up, I'm going to remove this. It breaks BC as lot of projects will have defined composer scripts to the original dev/task path.

chrispenny

Folks seem happy enough with the changes here. I've tested it on one of my [mammoth sized] projects, and it ran without issue.

If there are any further improvements, please don't hesitate to raise a new PR. I'm going to do my best to keep on top of things going forward.

jakxnz · 2021-10-20T20:17:32Z

code/Populate.php

 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Extensible;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Dev\YamlFixture;
 use SilverStripe\ORM\Connect\DatabaseException;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\DB;
+use SilverStripe\Versioned\Versioned;


Nitpick: It's plausible to use this module without the admin module (and/or the versioned module). So while this is a safe assumption (I'm guessing 99.9% of framework-based projects run admin), are we happy to make this assumption?

Oh, you'd already merged it. I overlooked that, sorry.

PHP allows you to reference classes (that don't exist) in this way without it breaking. Breakages would only occur when you attempt to use that class. My assumption is that that case is already covered, as all I did was change the reference.

Ah, I see. I had assumed if (!$obj->hasExtension(Versioned::class)) { is invoking the class, and so would throw an error

To be fair, I'm not not 100% sure about older versions of Extensible, but currently it's just running an is_a() on the instance and extension name.

Oh... Just noticed that Versioned is required. So we're good in any case.

🤦🏻 I probably should have checked that 😆

Support fluent

858fff5

jakxnz suggested changes Sep 4, 2020

View reviewed changes

jakxnz reviewed Sep 7, 2020

View reviewed changes

Support external truncation requirements via extension hook

f3e88ab

Merge pull request #1 from pjayme/feat/truncate-tables-extension-hook

6f59e34

Truncate tables extension hook

michalkleiner reviewed Sep 14, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

michalkleiner reviewed Sep 14, 2020

View reviewed changes

code/Populate.php Show resolved Hide resolved

Merge branch 'master' into feat/fluenty

726ed71

Update README - reinstate dev dep warning and fix confusing descriptions

d345858

chrispenny reviewed Oct 20, 2021

View reviewed changes

Minor tidy ups. Keep BC dev/task segment

23efe2d

chrispenny approved these changes Oct 20, 2021

View reviewed changes

chrispenny merged commit cdf58a5 into silverstripe:master Oct 20, 2021

jakxnz reviewed Oct 20, 2021

View reviewed changes

LiamKearn mentioned this pull request Feb 21, 2022

truncate_objects is not longer an array. #42

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support fluent and clean up #32

Support fluent and clean up #32

adrhumphreys commented Aug 7, 2020 •

edited

jakxnz left a comment

jakxnz Sep 4, 2020

jakxnz Sep 4, 2020 •

edited

madmatt Oct 12, 2021

adrhumphreys commented Sep 5, 2020

jakxnz Sep 6, 2020

jakxnz Sep 6, 2020

jakxnz commented Sep 7, 2020 •

edited

adrhumphreys commented Sep 11, 2020

jakxnz commented Sep 11, 2020

michalkleiner Sep 14, 2020

madmatt commented Oct 11, 2021

adrhumphreys commented Oct 12, 2021

jakxnz commented Oct 12, 2021 •

edited

madmatt commented Oct 13, 2021

madmatt commented Oct 13, 2021

michalkleiner commented Oct 13, 2021

jakxnz commented Oct 13, 2021 •

edited

chrispenny Oct 20, 2021

chrispenny left a comment

jakxnz Oct 20, 2021

jakxnz Oct 20, 2021

chrispenny Oct 20, 2021

jakxnz Oct 20, 2021

chrispenny Oct 20, 2021

chrispenny Oct 20, 2021 •

edited

jakxnz Oct 20, 2021

Support fluent and clean up #32

Support fluent and clean up #32

Conversation

adrhumphreys commented Aug 7, 2020 • edited

jakxnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakxnz Sep 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrhumphreys commented Sep 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakxnz commented Sep 7, 2020 • edited

adrhumphreys commented Sep 11, 2020

jakxnz commented Sep 11, 2020

Choose a reason for hiding this comment

madmatt commented Oct 11, 2021

adrhumphreys commented Oct 12, 2021

jakxnz commented Oct 12, 2021 • edited

madmatt commented Oct 13, 2021

madmatt commented Oct 13, 2021

michalkleiner commented Oct 13, 2021

jakxnz commented Oct 13, 2021 • edited

Choose a reason for hiding this comment

chrispenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispenny Oct 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrhumphreys commented Aug 7, 2020 •

edited

jakxnz Sep 4, 2020 •

edited

jakxnz commented Sep 7, 2020 •

edited

jakxnz commented Oct 12, 2021 •

edited

jakxnz commented Oct 13, 2021 •

edited

chrispenny Oct 20, 2021 •

edited