New Storage Layer - Add Compatibility features to allow both systems to work concurrently #6972

rossriley · 2017-09-04T05:15:21Z

This is another set of changes that allow themes to render correctly in TWIG no matter which storage layer provides the objects.

Main additions are moving the field type information into a Trait that can be shared by both objects, adding a Twig filter to post process new-style Taxonomy objects and adding some extra checks to the Frontend controller to handle potential differences between the two object types.

…nt object itself

Nitpick-CI · 2017-09-04T05:15:31Z

src/Storage/Entity/ContentTypeTrait.php

+        return $field['type'];
+    }
+
+}


The closing brace for the trait must go on the next line after the body

Nitpick-CI · 2017-09-04T05:15:32Z

src/Twig/Runtime/CompatRuntime.php

+        return $this->appConfig->get('general/compatibility/setcontent_legacy', true);
+    }
+
+}


The closing brace for the class must go on the next line after the body

GwendolenLynch · 2017-09-04T07:38:21Z

src/Twig/Extension/RecordExtension.php

+            new TwigFilter('current',           [Runtime\RecordRuntime::class, 'current']),
+            new TwigFilter('excerpt',           [Runtime\RecordRuntime::class, 'excerpt'], $safe),
+            new TwigFilter('selectfield',       [Runtime\RecordRuntime::class, 'selectField']),
+            new TwigFilter('trimtext',          [Runtime\RecordRuntime::class, 'excerpt'], $safe + $deprecated + ['alternative' => 'excerpt']),


Want me to git magic this out, or are you good with it?

GwendolenLynch · 2017-09-04T07:50:25Z

src/Storage/Entity/ContentTypeTrait.php

+     * @return array An associative array containing at least the key 'type',
+     *               and, depending on the type, other keys.
+     */
+    public function fieldinfo($key)


Can we CamelCase the name if this is going to live on outside of legacy?

GwendolenLynch · 2017-09-04T07:50:33Z

src/Storage/Entity/ContentTypeTrait.php

+     *
+     * @return string
+     */
+    public function fieldtype($key)


Can we CamelCase this too

GwendolenLynch · 2017-09-04T07:57:54Z

src/Twig/Runtime/RecordRuntime.php

+    {
+        $isLegacy = $this->config->get('general/compatibility/setcontent_legacy', true);
+
+    }


Accidental?

GwendolenLynch · 2017-09-04T08:00:25Z

src/Twig/Runtime/CompatRuntime.php

+    protected function isLegacy()
+    {
+        return $this->appConfig->get('general/compatibility/setcontent_legacy', true);
+    }


Wouldn't it cover more bases to drop this and instead check the type in taxonomyCompat()

GwendolenLynch · 2017-09-04T08:15:20Z

src/Twig/Extension/CompatExtension.php

+        $deprecated = ['deprecated' => true];
+
+        return [
+            new TwigFilter('taxonomy_compat', [Runtime\CompatRuntime::class, 'taxonomyCompat'], $safe + $deprecated)


OK, so digging I have a couple of question:

Why not just |taxonomy?

I know with 1. you've marked it up as deprecated, but is it, and what is the way forward?

Shouldn't this just go into the RecordRuntime?

I might still have a rethink about how to do this better, but yes, the problem it solves is that calling record.taxonomy on the legacy content objects gives a very specific key/value array structure that it's impossible to patch in without ruining the concept of having proper collections for taxonomies.

But at the same time, the taxonomy_links twig templates depend on them being this way, so I can't make it BC, so the next best option as far as I can think is to provide a filter to standardise until a time the theme author can rewrite the Twig.

So for this reason, it's new, but immediately deprecated so that we can remove in 4.0 by which time themes can have been tweaked to use the new structure.

Finally, I just thought it was better not to add the legacy config stuff into existing extensions but I don't mind either way, I started doing it in RecordRuntime and just thought it might be more isolated as a separate extension that we can throw away as soon as it's time has passed.

Oh totally in favour of a filter as opposed to trying to make it work as a method on the object, and that was the closest consensus we got to in previous meetings. Deity classes 🔥 👍

I was more thinking the naming of the filter, as people are getting increasingly narky at changes we make … that single point is my worry here. I know we are approaching this from "It is deprecated and will change", but I am just wondering after a few weeks of various people getting upset, it might be good to think about how we give them the way it will be to transition to, rather than one change now, and another at the end of the year … I don't have a solution either by the way, just trying to get the conversation happening to take some of this off you and I.

But if I am totally missing the point, please throw a wet rag at me.

GwendolenLynch · 2017-09-04T08:21:42Z

src/Controller/Frontend.php

@@ -443,6 +443,16 @@ public function template($template)
        return $this->render($template);
    }

+    public function isHome($content)


In before @CarsonF 😉 … But shouldn't this either be private or somewhere else. It just feels like more of the historical disasters we're trying to get away from.

Where do you reckon? It was in the actual content record before, which is a bit of a nightmare because we would have to inject all the config into the content entities.

That info is all available in the controller and it seems like the sort of thing that can/should be done there because you're using that info to decide which template to show etc.

Yeah, of everything I commented on here, this was the one that made my stomach churn as it is really hard … as above, my single worry is just public API (ab)use and the lack of flexibility in changing if we need to. If it is a helper it I'd guess a protected method in Base? … Please see my comment as looking to achieve a consensus with you too

cool, that sounds fair enough.

Only other strategy I can think of since the goal of this is to add some metadata to the request object is to do something with an event that can have the selected record passed in along with the $app['config'] and $app['request'] services passed in as dependencies.

If we did that though it would probably have to be up to the controller to trigger the event, unless we already have an event we could hook into.

since the goal of this is to add some metadata to the request object is to do something with an event

Actually, that's a bloody good idea!

I'm going to get slapped for the suggestion of location, but Bolt\EventListener\GeneralListener has to have the container passed due to needing to not construct Config early, so adding KernelEvents::REQUEST => 'onRequest', to the getSubscribedEvents would be totally feasible

Nitpick-CI · 2017-09-04T19:03:42Z

src/Twig/Runtime/CompatRuntime.php

+        return $compiled;
+    }
+
+}


The closing brace for the class must go on the next line after the body

Nitpick-CI · 2017-09-05T17:34:29Z

src/Controller/Base.php

+        $uriID = $content->contenttype['singular_slug'] . '/' . $content->get('id');
+        $uriSlug = $content->contenttype['singular_slug'] . '/' . $content->get('slug');
+
+        if (($uriID === $homepage || $uriSlug === $homepage) && ($template === $this->getOption('general/homepage_template')) ) {


Expected 0 spaces before closing bracket; 1 found

Nitpick-CI · 2017-09-08T10:35:44Z

src/Controller/Frontend.php

@@ -150,7 +150,7 @@ public function record(Request $request, $contenttypeslug, $slug = '')
        // First, try to get it by slug.
        $content = $this->getContent($contenttype['slug'], ['slug' => $slug, 'returnsingle' => true, 'log_not_found' => !is_numeric($slug)]);

-        if (!$content && is_numeric($slug)) {
+        if (is_numeric($slug) && (!$content || count($content)===0) ) {


Expected 0 spaces before closing bracket; 1 found

rossriley · 2017-09-08T11:49:20Z

So with a nod to #6985 should I drop the idea of the compatibility step and just start porting any necessary filters and functions into the Records twig extension?

That is move the taxonomy_compat filter here to a standard record|taxonomy filter that can look after formatting for Twig?

GwendolenLynch · 2017-09-08T11:57:33Z

So with a nod to #6985 should I drop the idea of the compatibility step and just start porting any necessary filters and functions into the Records twig extension?

WFM 👍 … That was kinda where I was heading with my review and discussion in Slack. Mainly so we don't have multiple changes that people have to deal with, we can give them something new to grab a hold of and use in 3.4+ and they can have confidence that their hard work won't be lost when they update to 4.x.

rossriley · 2017-09-08T14:59:50Z

ok, so this should be good to go.

I'm not going to stuff all those filters into this PR, just keep this focused on fixing taxonomy and isHome functionality, but this PR is enough to get a record page rendering identically on both sides of the toggle.

GwendolenLynch · 2017-09-08T15:02:05Z

Zero stress … I'll go over it in the morning as I think we all deserve to chill the "you know what" out and drink some 🍻 right now… 'cause Friday.

Translation: Thanks mate, have one on me! 👍

GwendolenLynch

Let's do this

rossriley added 3 commits September 3, 2017 16:47

implement isHome in a private method on the Base controller not conte…

b9e5a1e

…nt object itself

setup of new compat twig filters to ease transition between old and new

f6c9301

move fieldtype info to a trait and share between entities

0d79537

Nitpick-CI reviewed Sep 4, 2017

View reviewed changes

GwendolenLynch suggested changes Sep 4, 2017

View reviewed changes

bolt deleted a comment from Nitpick-CI Sep 4, 2017

Nitpick-CI reviewed Sep 4, 2017

View reviewed changes

src/Twig/Runtime/CompatRuntime.php Outdated

return $compiled;

}

}

Copy link

Nitpick-CI Sep 4, 2017

Choose a reason for hiding this comment

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

The closing brace for the class must go on the next line after the body

Nitpick-CI reviewed Sep 5, 2017

View reviewed changes

Nitpick-CI reviewed Sep 8, 2017

View reviewed changes

GwendolenLynch force-pushed the fix/move-ishome-to-controller branch from 7600cfb to 0d79537 Compare September 9, 2017 14:46

GwendolenLynch approved these changes Sep 9, 2017

View reviewed changes

GwendolenLynch merged commit b556ab3 into bolt:3.4 Sep 9, 2017

GwendolenLynch added this to the Bolt 3.4 - Feature release milestone Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Storage Layer - Add Compatibility features to allow both systems to work concurrently #6972

New Storage Layer - Add Compatibility features to allow both systems to work concurrently #6972

rossriley commented Sep 4, 2017

Nitpick-CI Sep 4, 2017

Nitpick-CI Sep 4, 2017

GwendolenLynch Sep 4, 2017

GwendolenLynch Sep 4, 2017

GwendolenLynch Sep 4, 2017

GwendolenLynch Sep 4, 2017

GwendolenLynch Sep 4, 2017

GwendolenLynch Sep 4, 2017 •

edited

rossriley Sep 4, 2017

GwendolenLynch Sep 4, 2017

GwendolenLynch Sep 4, 2017

rossriley Sep 4, 2017

GwendolenLynch Sep 4, 2017

rossriley Sep 4, 2017

GwendolenLynch Sep 4, 2017

Nitpick-CI Sep 4, 2017

Nitpick-CI Sep 5, 2017

Nitpick-CI Sep 8, 2017

rossriley commented Sep 8, 2017 •

edited

GwendolenLynch commented Sep 8, 2017

rossriley commented Sep 8, 2017

GwendolenLynch commented Sep 8, 2017

GwendolenLynch left a comment

New Storage Layer - Add Compatibility features to allow both systems to work concurrently #6972

New Storage Layer - Add Compatibility features to allow both systems to work concurrently #6972

Conversation

rossriley commented Sep 4, 2017

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

Choose a reason for hiding this comment

GwendolenLynch Sep 4, 2017 • edited

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

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

rossriley commented Sep 8, 2017 • edited

GwendolenLynch commented Sep 8, 2017

rossriley commented Sep 8, 2017

GwendolenLynch commented Sep 8, 2017

GwendolenLynch left a comment

Choose a reason for hiding this comment

GwendolenLynch Sep 4, 2017 •

edited

rossriley commented Sep 8, 2017 •

edited