Implemented page document and API #114

wachterjohannes · 2017-03-23T15:10:40Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Fixed tickets	none
Related issues/PRs	none
License	MIT
Documentation PR	none

What's in this PR?

This PR introduces a ArticlePageDocument. This documents will be saved as childs of ArticleDocuments.

Why?

This is the first part of the "Multi-Page" feature.

To Do

Unit-Tests
Indexing pages into article

wachterjohannes · 2017-03-24T08:16:05Z

Document/Subscriber/ArticleSubscriber.php

-     *
-     * @param AbstractMappingEvent $event
-     */
-    public function handleIndexLive(AbstractMappingEvent $event)


wasnt used anymore.

danrot

Looks good, pretty straight forward. Not sure about all these ONGR annotations though, maybe somebody else can have a look on them.

danrot · 2017-03-27T09:05:06Z

Controller/ArticlePageController.php

+            throw new ParameterNotAllowedException('title', ArticlePageDocument::class);
+        }
+        if (array_key_exists('template', $data)) {
+            throw new ParameterNotAllowedException('template', ArticlePageDocument::class);


At least until now we always ignored extra parameters... There's the same happening for the id in many controllers in Sulu core... Not sure if we should start handling it here differently, resp. what would be the right way to handle this.

the problem here is that this has a different meaning now title is the article-title and it cannot be changed over this controller.

danrot · 2017-03-27T09:20:43Z

Document/Form/ArticleDocumentType.php

@@ -32,17 +32,12 @@ public function buildForm(FormBuilderInterface $builder, array $options)
        // extensions
        $builder->add('extensions', TextType::class, ['property_path' => 'extensionsData']);

-        // TODO: Fix the admin interface to not send this junk (not required for articles)


Was this fix as easy as just removing these lines?

no (: the fix is:

public function configureOptions(OptionsResolver $resolver) { $resolver->setDefaults( [ 'allow_extra_fields' => true, ] ); }

which is done since a while in this class.

danrot · 2017-03-27T09:36:47Z

Document/Subscriber/ArticlePageSubscriber.php

+            return;
+        }
+
+        $pageTitle = uniqid('page-', true);


Isn't that a strange page title? Shouldn't we fix that in a proper way? Looks like you are circumventing another problem here.

it could be that the articles dont have a "pageTitle" attribute. it should not be mandatory. then we have to generate a "unique" node-name. this could be "Page 1", "Page 2" ... but that would mean that we have to update them on add and delete.

danrot · 2017-03-27T09:41:15Z

Document/Subscriber/ArticleSubscriber.php

@@ -90,7 +96,11 @@ public function handleScheduleIndex(AbstractMappingEvent $event)
    {
        $document = $event->getDocument();
        if (!$document instanceof ArticleDocument) {
-            return;
+            if (!$document instanceof ArticlePageDocument) {
+                return;


Just for my understanding: Is this case still possible? Or put differently: Can we still save ArticleDocuments, or are we always saving ArticlePageDocuments from now on?

both is possible. The ArticleDocument is the first page and all the other pages has its own document.

danrot · 2017-03-27T09:42:15Z

Document/Subscriber/ArticleSubscriber.php

-            );
+        foreach ($this->documents as $documentData) {
+            $document = $this->documentManager->find($documentData['uuid'], $documentData['locale']);
+            $this->documentManager->refresh($document, $documentData['locale']);


Why is this refresh now necessary?

if you add a new page during the session you have to refresh the children property to be sure that you index the newest data.

danrot · 2017-03-27T09:50:00Z

Resources/config/services.xml

@@ -122,14 +128,18 @@
            <!-- This needs to happen after the content repository has been initialized !-->
            <tag name="sulu_document_manager.initializer" priority="-127"/>
        </service>
-        <service id="sulu_article.serializer.type"
+        <service id="sulu_article.serializer.article"


Mind that this is a BC break. Maybe we can add the public="false" attribute, so that the BC break is not that bad (although it's still there, it is only less likely to occur).

i will add the public=false flag.

shit (: this does not work - subscriber has to be public https://travis-ci.org/sulu/SuluArticleBundle/jobs/215473502#L491

danrot · 2017-03-27T09:50:08Z

Resources/config/services.xml

                 class="Sulu\Bundle\ArticleBundle\Document\Serializer\ArticleSubscriber">
            <argument type="service" id="sulu.content.structure_manager"/>
            <argument type="service" id="sulu.content.type_manager"/>
            <argument type="service" id="sulu_article.content.data_provider.proxy_factory"/>

            <tag name="jms_serializer.event_subscriber" />
        </service>
+        <service id="sulu_article.serializer.article_page"


Could also be private?

can add it also here

shit (: this does not work https://travis-ci.org/sulu/SuluArticleBundle/jobs/215473502#L491

danrot · 2017-03-27T09:51:12Z

Resources/doc/installation.md

@@ -36,10 +36,14 @@ sulu_core:
        structure:
            default_type:
                article: "article_default"
+                article_page: "article_default"


This also confuses me... Are these two types basically representing the same, and the only difference is that the article represents the first page only?

yes (: the configuration is a bit strange ... maybe find a better way to configure it later.

wachterjohannes · 2017-03-27T10:16:13Z

@danrot done

alexander-schranz · 2017-03-29T11:59:32Z

Controller/ArticlePageController.php

+     */
+    public function getSecurityContext()
+    {
+        return ArticleAdmin::SECURITY_CONTEXT;


* implemented basic page document * added index pages to article

* Implemented page document and API (#114) * implemented basic page document * added index pages to article * fixed serialization context of article and article-page (#118) * Add interface to manage article-pages (#116) * introduced css grunt job * added article-router * added page dropdown * added logic to create and update pages * renamed adapter * Added article title to page form (#123) * added article title * fixed selector for css stye * fixed jquery selector of grid-row * added page to content-navigation (#125) * Fixed create article from ghost (#126) * fixed create article from ghost * hide new page link for new articls * fixed code style * added delete page button (#132) * added translations (#128) * added route-behavior to article-page (#131) * Fixed priorities of subscriber (#137) * fixed priorities of subscriber * added tag to find routePath property * Fixed title property (article-title) and add page title property (#141) * fixed title property (article-title) and add page title property * added node-name-slugifier * Changed generating route only on publish (#138) * use publish event to generate routes * refactored because of sulu changes * Added page-number argument to website-article-controller (#142) * added serialization of pages * added pages data to article-document * fixed comments * Added synchronize children between draft and live (#139) * added synchronice children between draft and live * fixed preview for new pages * fixed serialization for preview * fixed testcases * Fixed versioning and remove-draft of article-pages (#145) * fixed versioning of article-pages * added upgrade note * added constraint for jackalope/jackalope * implemented duplicate article (#146) * Fixed preview-serialization of article pages (#154) * fixed preview-serialization of article pages * change view of renderArticle * fixed type change on list (#156) * Fixed metadata for article to sync remove (#155) * fixed metadata for article * fixed routable-subscriber to remove all routes (locales) * removed article-page copy locale (#157) * fixed code-style * fixed test-setup

wachterjohannes force-pushed the feature/multi-page branch 6 times, most recently from 5c04212 to 5539d04 Compare March 24, 2017 08:03

implemented basic page document

6ad89af

wachterjohannes force-pushed the feature/multi-page branch from 5539d04 to db430f4 Compare March 24, 2017 08:09

wachterjohannes commented Mar 24, 2017

View reviewed changes

wachterjohannes force-pushed the feature/multi-page branch 2 times, most recently from a03672a to 038cb11 Compare March 24, 2017 11:58

added index pages to article

767004f

wachterjohannes force-pushed the feature/multi-page branch from 038cb11 to 767004f Compare March 27, 2017 06:17

wachterjohannes requested a review from danrot March 27, 2017 08:27

danrot suggested changes Mar 27, 2017

View reviewed changes

wachterjohannes force-pushed the feature/multi-page branch from 0b88ef1 to 767004f Compare March 27, 2017 13:41

wachterjohannes mentioned this pull request Mar 28, 2017

Improved serialization context of article and article-page #118

Merged

wachterjohannes requested a review from alexander-schranz March 29, 2017 08:41

alexander-schranz reviewed Mar 29, 2017

View reviewed changes

alexander-schranz approved these changes Mar 29, 2017

View reviewed changes

alexander-schranz merged commit 48a8251 into sulu:feature/multi-page Mar 29, 2017

wachterjohannes added a commit to wachterjohannes/SuluArticleBundle that referenced this pull request Mar 29, 2017

Implemented page document and API (sulu#114)

aba25a9

* implemented basic page document * added index pages to article

wachterjohannes added a commit to wachterjohannes/SuluArticleBundle that referenced this pull request Mar 31, 2017

Implemented page document and API (sulu#114)

a50ae6e

* implemented basic page document * added index pages to article

wachterjohannes added a commit to wachterjohannes/SuluArticleBundle that referenced this pull request Apr 5, 2017

Implemented page document and API (sulu#114)

6972f1f

* implemented basic page document * added index pages to article

wachterjohannes added a commit that referenced this pull request Apr 14, 2017

Implemented page document and API (#114)

80f386d

* implemented basic page document * added index pages to article

wachterjohannes added a commit to wachterjohannes/SuluArticleBundle that referenced this pull request Apr 20, 2017

Implemented page document and API (sulu#114)

b4d7192

* implemented basic page document * added index pages to article

wachterjohannes added a commit to wachterjohannes/SuluArticleBundle that referenced this pull request Apr 21, 2017

Implemented page document and API (sulu#114)

343cd1b

* implemented basic page document * added index pages to article

wachterjohannes added a commit to wachterjohannes/SuluArticleBundle that referenced this pull request Apr 24, 2017

Implemented page document and API (sulu#114)

829848c

* implemented basic page document * added index pages to article

wachterjohannes added a commit to wachterjohannes/SuluArticleBundle that referenced this pull request Apr 25, 2017

Implemented page document and API (sulu#114)

29ed773

* implemented basic page document * added index pages to article

wachterjohannes added a commit to wachterjohannes/SuluArticleBundle that referenced this pull request Apr 25, 2017

Implemented page document and API (sulu#114)

4311557

* implemented basic page document * added index pages to article

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented page document and API #114

Implemented page document and API #114

wachterjohannes commented Mar 23, 2017 •

edited

Loading

wachterjohannes Mar 24, 2017

danrot left a comment

danrot Mar 27, 2017

wachterjohannes Mar 27, 2017

danrot Mar 27, 2017

wachterjohannes Mar 27, 2017

danrot Mar 27, 2017

wachterjohannes Mar 27, 2017

danrot Mar 27, 2017

wachterjohannes Mar 27, 2017 •

edited

Loading

danrot Mar 27, 2017

wachterjohannes Mar 27, 2017

danrot Mar 27, 2017

wachterjohannes Mar 27, 2017

wachterjohannes Mar 27, 2017

danrot Mar 27, 2017

wachterjohannes Mar 27, 2017

wachterjohannes Mar 27, 2017

danrot Mar 27, 2017

wachterjohannes Mar 27, 2017

wachterjohannes commented Mar 27, 2017

alexander-schranz Mar 29, 2017

Implemented page document and API #114

Implemented page document and API #114

Conversation

wachterjohannes commented Mar 23, 2017 • edited Loading

What's in this PR?

Why?

To Do

Choose a reason for hiding this comment

danrot 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wachterjohannes Mar 27, 2017 • edited Loading

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

wachterjohannes commented Mar 27, 2017

Choose a reason for hiding this comment

wachterjohannes commented Mar 23, 2017 •

edited

Loading

wachterjohannes Mar 27, 2017 •

edited

Loading