[PagePartBundle] refactor PagePartReader and PageTemplateReader allowing using symfony configuration for storage

[mlebkowski]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Fixed tickets	-

The Goal

This is a general cleanup of the KunstmaanPagePartBundle with the main goal of extracting the PagePartConfigurationReader into a service implementing an interface (this way it could be easily extended or replaced).

There have been instances that I wanted to alter the way the pageTemplate is selected, etc. Having the class instance created in the methods body prevented me from extending it in any way.

New configuration method

The new feature is the ability to define pageparts and pagetemplates via the symfony configuration (the configuration values are exactly the same as in the yml files now), i.e.:

# app/config/config.yml
kunstmaan_page_part:
  pageparts: 
     article: … # same content as in `Resources/config/pageparts/article.yml`
   pagetemplates:
      homepage: …

This of course has the advantage of caching, validation, etc.

Breaking changes and stability

The breaking changes are described in the UPGRADE-X.X.md file. It’s hard to tell if they are even breaking, since they only touch on the internals, so I have mixed feelings about them. Anyway, they won’t affect users that didn’t tweak with the bundle mechanisms, i.e. a "StandardEdition" website will not break.

In the meantime I’m switching to this version in my new under-development website so I can spot any bugs that could be introduced, so this should be battle tested by the time 3.6 will be released. You don’t have a 3.6 branch yet, so I’m merging to master.

In closing

Do you have any feedback for me regarding this type of changes? I’d like to invest more time into cleanups like this — both keeping BC and breaking it for some future 4.0 release — but it would be a shame if they do not align with your roadmap.

[jockri]

Thanks for your contribution! We'll take a closer look to your refactoring later on. I'll keep you posted!

[Numkil]

This line is making me crash whenever I try to visit any of the pages to edit them. For example on url /en/admin/nodes/2. However I see in the php documentation for the method explode that the delimeter should be the first argument and the string the second argument.

So I tried changing this line in explode(':', $name, 2); and now everything seems to work as expected again.

[mlebkowski]

@Numkil, I included your suggestions in 0eb69d7.

Of course it was my error with the parameters order, good catch

[Numkil]

Probably not relevant here but using in_array in a loop could potentially be extremely slow if the array starts growing a lot. In this case it's probably smarter to append $context to $result unconditionally and return array_keys(array_flip($result));

But as I said it's probably a non-issue.

[mlebkowski]

public function getPagePartContexts(HasPagePartsInterface $page) 
{
        $result = [];

        foreach ($this->getPagePartAdminConfigurators($page) as $configurator) {
            $result[$configurator->getContext()] = true;
        }

        return array_keys($result);
}

hm?

[Numkil]

that's even better.

[Numkil]

+1 Many useful improvements in the code. I wasn't able to find any other errors on the webpage so I assume it works quite well. @jockri

[mlebkowski]

Guys, honestly, that is the response I was anticipating. I offered an in-depth involvement and quality contributions, but you don’t seem to be interested. In the meantime you didn’t hesitate to merge a conflicting PR, giving me additional work if I wanted to get this merged in…

[jockri]

Hi @mlebkowski , sure we are interested and thankful for your great contributions, let that be clear!
There is indeed a small PR merged that added an extra feature/option...
Can you rebase so we can merge before there are more conflicts?
Thanks!

[jockri]

Thanks!