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

[WIP] Feature/hierarchical content #7227

Open
wants to merge 6 commits into
base: 3.x
Choose a base branch
from

Conversation

mrenigma
Copy link

@mrenigma mrenigma commented Dec 2, 2017

So here is the long-awaited PR!

This PR implements a hierarchy system in Bolt which allows individual content types to be defined as hierarchical or not. If a content type is hierarchical, it will be reflected in the routing layer, it will be given a parent field to define the record parent (restricted to the same content type) and it will add additional methods to the record to retrieve its hierarchy, its children and it's root parent record.

This implementation was originally built for a client site, so there may be methods or processes which don't need to happen in core Bolt!

How to make it work

To make hierarchical work you have to add the following to a content type in contenttypes.yml:

hierarchical: true

And add/edit the following in the router.yml :

contentlink:
    path: /{contenttypeslug}/{slug}
    requirements:
        contenttypeslug: controller.requirement:anyContentType
        slug: '[a-z0-9-_\/]+'

This is to allow / in the routing structure (may be possible to automate?).

Still to do/issues

  • Move parent-change.js into the Bolt core JS files. I've started work on this but I'm definitely no expert on the Bolt backend assets pipeline, so any help would be greatly appreciated! 4557cf7.
  • Once that is done, resolve this TODO and remove.
  • Consider whether using the JSON approach from the TODO above is worth replacing with an AJAX call instead.
  • Style the listing pages nicely, especially when several levels deep or in groups.
  • Decide whether to ignore pagination when displaying hierarchical content (i.e. show more than 10) or to try and make that work too.
  • Update "View saved version" link on update of the parent field (this actually doesn't work in non-hierarchical Bolt either, changing the slug doesn't update that link!).
  • Make sure the URL field on records is updating on change of parent and the full hierarchical URL is visible on record page load.
  • Check and clean up any code style issues (FYI an editor.config in Bolt would be amazingly helpful in future)
  • Add to tests to all new methods and changes to existing ones

src/Config.php Outdated
}

// Insert the parentid field into the $slugPos
$contentType['fields'] = array_merge(array_slice($contentType['fields'],0, $slugPos), $parent, array_slice($contentType['fields'], $slugPos));

Choose a reason for hiding this comment

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

No space found after comma in function call

->setLate(true);

$snippet = (new Snippet())
->setCallback('<script>var hierarchies = JSON.parse(\'' . json_encode($this->app['storage.hierarchy']->getAllHierarchies($contenttypeslug, false),

Choose a reason for hiding this comment

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

Opening parenthesis of a multi-line function call must be the last content on the line


$snippet = (new Snippet())
->setCallback('<script>var hierarchies = JSON.parse(\'' . json_encode($this->app['storage.hierarchy']->getAllHierarchies($contenttypeslug, false),
JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_QUOT | JSON_HEX_AMP | JSON_UNESCAPED_UNICODE) . '\');</script>')

Choose a reason for hiding this comment

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

  • Multi-line function call not indented correctly; expected 16 spaces but found 20
  • Closing parenthesis of a multi-line function call must be on a line by itself

src/Config.php Outdated
"The identifier and slug for '%taxonomytype%' are the not the same ('%slug%' vs. '%taxonomytype%'). Please edit taxonomy.yml, and make them match to prevent inconsistencies between database storage and your templates.",
['%taxonomytype%' => $key, '%slug%' => $taxo['slug']]
);
$error = Trans::__("The identifier and slug for '%taxonomytype%' are the not the same ('%slug%' vs. '%taxonomytype%'). Please edit taxonomy.yml, and make them match to prevent inconsistencies between database storage and your templates.",

Choose a reason for hiding this comment

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

Opening parenthesis of a multi-line function call must be the last content on the line

src/Config.php Outdated
[
'%taxonomytype%' => $key,
'%slug%' => $taxo['slug']
]);

Choose a reason for hiding this comment

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

Closing parenthesis of a multi-line function call must be on a line by itself

src/Config.php Outdated
'quality' => 75,
'cropping' => 'crop',
'notfound_image' => 'bolt_assets://img/default_notfound.png',
'error_image' => 'bolt_assets://img/default_error.png',
'only_aliases' => false,
],
'accept_file_types' => explode(',', 'twig,html,js,css,scss,gif,jpg,jpeg,png,ico,zip,tgz,txt,md,doc,docx,pdf,epub,xls,xlsx,csv,ppt,pptx,mp3,ogg,wav,m4a,mp4,m4v,ogv,wmv,avi,webm,svg'),
'accept_file_types' => explode(',',

Choose a reason for hiding this comment

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

Opening parenthesis of a multi-line function call must be the last content on the line

src/Config.php Outdated
'quality' => 75,
'cropping' => 'crop',
'notfound_image' => 'bolt_assets://img/default_notfound.png',
'error_image' => 'bolt_assets://img/default_error.png',
'only_aliases' => false,
],
'accept_file_types' => explode(',', 'twig,html,js,css,scss,gif,jpg,jpeg,png,ico,zip,tgz,txt,md,doc,docx,pdf,epub,xls,xlsx,csv,ppt,pptx,mp3,ogg,wav,m4a,mp4,m4v,ogv,wmv,avi,webm,svg'),
'accept_file_types' => explode(',',
'twig,html,js,css,scss,gif,jpg,jpeg,png,ico,zip,tgz,txt,md,doc,docx,pdf,epub,xls,xlsx,csv,ppt,pptx,mp3,ogg,wav,m4a,mp4,m4v,ogv,wmv,avi,webm,svg'),

Choose a reason for hiding this comment

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

  • Multi-line function call not indented correctly; expected 12 spaces but found 16
  • Closing parenthesis of a multi-line function call must be on a line by itself

src/Config.php Outdated
@@ -1152,7 +1237,8 @@ protected function getDefaults()
'x_frame_options' => true,
],
'htmlcleaner' => [
'allowed_tags' => explode(',', 'div,span,p,br,hr,s,u,strong,em,i,b,li,ul,ol,mark,blockquote,pre,code,tt,h1,h2,h3,h4,h5,h6,dd,dl,dh,table,tbody,thead,tfoot,th,td,tr,a,img,address,abbr,iframe'),
'allowed_tags' => explode(',',

Choose a reason for hiding this comment

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

Opening parenthesis of a multi-line function call must be the last content on the line

src/Config.php Outdated
@@ -1152,7 +1237,8 @@ protected function getDefaults()
'x_frame_options' => true,
],
'htmlcleaner' => [
'allowed_tags' => explode(',', 'div,span,p,br,hr,s,u,strong,em,i,b,li,ul,ol,mark,blockquote,pre,code,tt,h1,h2,h3,h4,h5,h6,dd,dl,dh,table,tbody,thead,tfoot,th,td,tr,a,img,address,abbr,iframe'),
'allowed_tags' => explode(',',
'div,span,p,br,hr,s,u,strong,em,i,b,li,ul,ol,mark,blockquote,pre,code,tt,h1,h2,h3,h4,h5,h6,dd,dl,dh,table,tbody,thead,tfoot,th,td,tr,a,img,address,abbr,iframe'),

Choose a reason for hiding this comment

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

  • Multi-line function call not indented correctly; expected 16 spaces but found 20
  • Closing parenthesis of a multi-line function call must be on a line by itself

src/Config.php Outdated
'<pre>' . htmlspecialchars($this->cacheFile->getFullPath()) . '</pre>' .
'<p>' . str_replace('<a>', '<a href="javascript:history.go(-1)">', $part) . '</p>';
$part = Translator::__('Try logging in with your ftp-client and make the file readable. ' . 'Else try to go <a>back</a> to the last page.');
$message = '<p>' . Translator::__('general.phrase.file-not-readable-following-colon') . '</p>' . '<pre>' . htmlspecialchars($this->cacheFile->getFullPath()) . '</pre>' . '<p>' . str_replace('<a>',

Choose a reason for hiding this comment

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

Opening parenthesis of a multi-line function call must be the last content on the line

src/Config.php Outdated
'<p>' . str_replace('<a>', '<a href="javascript:history.go(-1)">', $part) . '</p>';
$part = Translator::__('Try logging in with your ftp-client and make the file readable. ' . 'Else try to go <a>back</a> to the last page.');
$message = '<p>' . Translator::__('general.phrase.file-not-readable-following-colon') . '</p>' . '<pre>' . htmlspecialchars($this->cacheFile->getFullPath()) . '</pre>' . '<p>' . str_replace('<a>',
'<a href="javascript:history.go(-1)">', $part) . '</p>';

Choose a reason for hiding this comment

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

  • Multi-line function call not indented correctly; expected 12 spaces but found 20
  • Only one argument is allowed per line in a multi-line function call
  • Closing parenthesis of a multi-line function call must be on a line by itself

@jkazimir
Copy link
Contributor

jkazimir commented Dec 2, 2017

Wow! :-)

Having a look, and noticed there's a ton of cache files here adding to the already long diff. Probably shouldn't have been committed?

@mrenigma
Copy link
Author

mrenigma commented Dec 2, 2017

@jkazimir thanks, didn't even notice those! I'll remove them from git

@bobdenotter
Copy link
Member

Awesome! Will check it out soon to monkey-test. I do notice quite a bit of the changes are whitespacing and code-style things. But, we can fix those. :-)

@mrenigma
Copy link
Author

mrenigma commented Dec 2, 2017

Yeah I know. My PHPStorm isn't set up in Bolt style. I tried setting up the code style stuff but it didn't work initially. I haven't had time to have another go since.
I've done my best to not save and format to try and avoid the style differences but things have probably slipped through the net.

.gitignore Outdated
@@ -54,4 +54,5 @@ Vagrantfile
.swp
*.lock
.buildpath
.idea
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Would you like me to remove this? I saw you had quite a few IDE specific ignores but not PHPStorm so I added it but I can remove

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'll catch this and a few other things in the rebase we talked about in Slack just now 😄

Copy link
Contributor

@GwendolenLynch GwendolenLynch left a comment

Choose a reason for hiding this comment

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

Also, we'll need to rebase out the cache commits, revert some of the removals and the spacing

@@ -31,6 +32,10 @@ public function editlink()
{
$perm = 'contenttype:' . $this->contenttype['slug'] . ':edit:' . $this->id;

if (empty($this->app)) {
$this->app = AppSingleton::get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Must not be used, ever 😉

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm I swear I copied this from somewhere else in Bolt 🤔. Doesn't matter, I'll update all of these to remove the empty!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it is not the empty() but the use of the application object in the entity … we'll chat about it in Slack in the coming week when I am back in NL

Copy link
Author

Choose a reason for hiding this comment

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

Not the empty()?! 😲 I never thought I'd hear you say that 😜. Okay sure, let's chat once your back from NL.

@@ -48,10 +53,38 @@ public function editlink()
public function link($referenceType = UrlGeneratorInterface::ABSOLUTE_PATH)
{
list($name, $params) = $this->getRouteNameAndParams();

if (empty($this->app)) {
$this->app = AppSingleton::get();
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

@@ -61,6 +94,10 @@ public function link($referenceType = UrlGeneratorInterface::ABSOLUTE_PATH)
*/
public function isHome()
{
if (empty($this->app)) {
$this->app = AppSingleton::get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@@ -133,10 +170,26 @@ public function getRouteNameAndParams()
*/
protected function getRouteConfig()
{
if (empty($this->app)) {
$this->app = AppSingleton::get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Here also

public function __construct($app)
{

$this->app = $app;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't inject the application, pass the services you need instead

@GwendolenLynch GwendolenLynch force-pushed the feature/hierarchical-content branch 3 times, most recently from 2fbc100 to 1c77c54 Compare December 3, 2017 15:25
@graham73may
Copy link
Contributor

@mrenigma, anythings I can do on this PR to get the ball rolling again? If you ELI5 I might get somewhere 😉

@mrenigma
Copy link
Author

@graham73may please do! I've been swamped (as you know). All the to-dos are on this PR so take your pick. If you see something you think you could help with, let me know and I'll do my best to try and explain it.

@graham73may
Copy link
Contributor

graham73may commented Jan 13, 2018 via email

@mrenigma
Copy link
Author

ContentRouteTrait is tied to the entity changes being done. Not sure what the core teams plans are on this one.

On the Hierarchy.php the app needs to be changed to inject the services from app instead. So rather than $this->app['service'] it should be passed so it's used as $this->service.

There are several services being initiated around the hierarchy initiation so plenty of examples of how to do it there 😄

@bobdenotter
Copy link
Member

@graham73may @mrenigma Hey guys, making progress on this one? :-)

@mrenigma
Copy link
Author

@bobenotter honestly I haven't touched it. I'm currently switching jobs and working overtime during the transition. I don't know if @graham73may has had a chance?

@graham73may
Copy link
Contributor

No news from me.

I think it would be good to get some help from the Bolt Core team on how the tasks in the PR get implemented:

JS

  • Move parent-change.js into the Bolt core JS files. I've started work on this but I'm definitely no expert on the Bolt backend assets pipeline, so any help would be greatly appreciated! 4557cf7.
  • Once that is done, resolve this TODO and remove.
  • Consider whether using the JSON approach from the TODO above is worth replacing with an AJAX call instead.

Backend Styling

  • Style the listing pages nicely, especially when several levels deep or in groups.
  • Decide whether to ignore pagination when displaying hierarchical content (i.e. show more than 10) or to try and make that work too.

Backend issues / bugs

  • Update "View saved version" link on update of the parent field (this actually doesn't work in non-hierarchical Bolt either, changing the slug doesn't update that link!).
  • Make sure the URL field on records is updating on change of parent and the full hierarchical URL is visible on record page load.

Housework

  • Check and clean up any code style issues (FYI an editor.config in Bolt would be amazingly helpful in future)
  • Add to tests to all new methods and changes to existing ones

@rossriley
Copy link
Contributor

In terms of approach I'd suggest that we get the new storage updates merged in soon and start work on the 3.5 release process. Then we get this PR rebased onto 3.6 and get it merged in. That means I can jump in and do some work on it and means we'll have a month or two to get everything solid before the 3.6 release.

@bobdenotter
Copy link
Member

Hi, @mrenigma and @graham73may,

Has there been any recent progress on this issue?

@graham73may
Copy link
Contributor

Hi @bobdenotter

This has been sat on the shelf whilst we were waiting for the new storage layer changes to be merged in (from Ross's last comment).

To be honest, we'd feel a lot better if someone could help us with the backend Javascript and CSS stuff, we're not familiar with any of this at all and Grunt was being a bit of a pig the last time we tried it.

We are VERY keen to get this in to core as soon as possible, is anyone of the team available to help push this forward from your end?

@rossriley
Copy link
Contributor

I'm happy to look after getting this into core, just needed to get the storage PR in first, since this can hopefully be one of the cool features that persuades people to switch the old layer off.

@bob I might need some input on the UI side but we can iron those things out at the testing phase.

One suggestion for discussion, I wondered whether in choosing the field names that end users set in contenttypes, we might pick something simpler to spell than hierarchical and parentid I was thinking perhaps we could go just with type: parent and type: child or maybe tree and child .. just thought that might save some typos in people's day to day use but any other thoughts?

@bobdenotter
Copy link
Member

I'll happily help out on the UI side of things! Let's make proper work of this, when the Storage PR lands! 👍

@graham73may
Copy link
Contributor

The Storage PR is tagged as 3.6.... Any chance this would fall under 3.6 too? 👼

@stale stale bot added the stale Stale issues & PRs flagged for closing label Jun 17, 2018
@GwendolenLynch GwendolenLynch added keep and removed stale Stale issues & PRs flagged for closing labels Jun 18, 2018
@bolt bolt deleted a comment from stale bot Jun 18, 2018
@JarJak
Copy link
Member

JarJak commented Nov 26, 2018

@bobdenotter @rossriley how about implementing this one in v4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants