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

EZP-28009: Created RichText FieldType Bundle #1

Merged
merged 10 commits into from Feb 14, 2018

Conversation

6 participants
@alongosz
Copy link
Member

commented Feb 13, 2018

This PR introduces basic skeleton for RichText FieldType Bundle.
This is only the first step towards EZP-28009, it allows adding the Bundle to AppKernel (with DI config placeholder).

The directory structure is as follows:

.
+-- src
|   +-- bundle
|   +-- lib # not available yet, future PR will introduce it
+-- tests # also not available yet
|   +-- lib
|   +-- bundle

Introduced:

  • Bundle class.
  • Extension class with injected placeholder for services configuration (services.yml).
  • README, LICENSE.
  • Composer (needed also by Packagist for README installation instructions to work).
  • php-cs-fixer configuration
  • Initial PHPUnit configuration.

Please note that links included in README will not work yet as adding all Resources from ezpublish-kernel RichText FieldType would clutter this PR diff.

],
'yoda_style' => false,
'no_break_comment' => false,
'declare_strict_types' => true,

This comment has been minimized.

Copy link
@alongosz

alongosz Feb 13, 2018

Author Member

Note: this is new in our bundles, along with single_blank_line_before_namespace => true it enforces

declare(strict_types=1);

namespace EzSystems\EzPlatformRichTextFieldTypeBundle\DependencyInjection;

after the licensing comment in each PHP file.

@@ -1,2 +1,84 @@
# ezplatform-richtext-fieldtype

This comment has been minimized.

Copy link
@alongosz

alongosz Feb 13, 2018

Author Member

@DominikaK README review-please-ping ;)

@alongosz alongosz requested review from Nattfarinn, webhdx and DominikaK Feb 13, 2018

},
"extra": {
"branch-alias": {
"dev-master": "0.1.x-dev"

This comment has been minimized.

Copy link
@lserwatka

lserwatka Feb 13, 2018

Member

Please start from 1.0.x-dev, Rich Text is mature.

This comment has been minimized.

Copy link
@alongosz

alongosz Feb 13, 2018

Author Member

@lserwatka fixed 👍

@alongosz alongosz force-pushed the alongosz:master branch from 1e10e3b to 0aa5692 Feb 13, 2018

@lserwatka

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

We need CI setup for this repo. Ping help @mnocon

README.md Outdated
## Background

When looking to find a structured text format for eZ Platform, we wanted to pick something that
was wildly used in the industry and which could support the custom & embed structures we have

This comment has been minimized.

Copy link
@DominikaK

DominikaK Feb 14, 2018

widely used, not wildly ;)

This comment has been minimized.

Copy link
@alongosz

alongosz Feb 14, 2018

Author Member

@DominikaK thanks, amended in d271944

@andrerom

This comment has been minimized.

Copy link
Member

commented Feb 14, 2018

Will we be able to import git history (git subtree split magic as Bertrand and others have done before) of the field type from kernel still even if we make the skeleton first? (Context: We frequently need the history for support to go back and see why or when something was changed)

@alongosz alongosz force-pushed the alongosz:master branch from 0aa5692 to dfc43d3 Feb 14, 2018

@alongosz

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2018

Will we be able to import git history (git subtree split magic as Bertrand and others have done before) of the field type from kernel still even if we make the skeleton first? (Context: We frequently need the history for support to go back and see why or when something was changed)

First of all, I haven't done that before, so I might be wrong, but I see several problems with that:

  1. The structure of the Bundle is different, please notice that we don't have EzRichTextFieldTypeBundle in ezpublish-kernel. I moved files around.
  2. AFAICS subtree is meant to split active and maintained part of a project as a subdirectory. Because of [1.] this is not really a subdirectory.
  3. Core/FieldType/RichText have grown over the years, I intend to do a cleanup (more moving files around, e.g. utilizing Sf 3.4+ DI Container configuration features).
  4. Namespaces are different, so that would need to be changed in moved files anyway.
  5. Related to [2.] and [3.] the Bundle code is intended to diverge from what we have in Core/FieldType/RichText namespace. The Core/FieldType/RichText namespace is going to be deprecated. AFAICS Subtree is something that can be pulled-in when upstream changes. This is not the case here.
  6. Doing a split would mean having files initially put in a wrong place (the code wouldn't work) and then applying proper changes as a separate commits. IMHO it would rather clutter history.

Summary: I'm not a huge fan of the idea. I understand advantages provided by continuous history, however IMHO that would lead to cluttering it and applying more work trying to cleanup instead of introducing a correct code in the first place.
Moreover, if I read the Git subtree doc right, while it is possible to diverge a subtree that has been split, it's rather intented for merges, not for code that somewhat introduces new stack / structure.

POV ping @lserwatka @bdunogier @Nattfarinn

@andrerom

This comment has been minimized.

Copy link
Member

commented Feb 14, 2018

  1. this is possible with some additional magic.
    2, magic, lots of things is possible, including extracting several subtrees and importing them in a common code base
  2. can be done after the import
  3. same, but can maybe also be rewritten during the split afaik, which would be cleaner as it will then adapt the history afaik
  4. In a split scenario we remove Core/FieldType/RichText and have BC note that the field type has been moved out to own package with adjusted namespace.
  5. some history is better then none, initial commit blobs are the worst

Don't dismiss the magic you don't believe to exist or don't have knowledge of, magic is real 🎆 😄
In other words, talk to @bdunogier here.

@lserwatka lserwatka merged commit 924dd23 into ezsystems:master Feb 14, 2018

@Nattfarinn

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2018

"Any sufficiently advanced technology is indistinguishable from magic."
-- Arthur C. Clarke

@bdunogier

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

To be honest, unless we figure out how to keep history without spending days on it, I'm not gonna insist on it.

The history does exist, inside eZ Publish kernel. Maybe the commits where the code is moved from the kernel could have a link to the reference this was splitted from ? And the branch it still lives in. That way, we can still go through history, even if there's an extra jump.

If we really want to make it happen, then we're gonna have to get creative :)
Maybe it would be simpler to do the opposite: prune from ezpublish-kernel everything that it NOT richtext, and keep the history for what's left.

Then, merge that into this repository (or use it as the initial commit of it).

@alongosz

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

Maybe it would be simpler to do the opposite: prune from ezpublish-kernel everything that it NOT richtext, and keep the history for what's left.

Filtering branch is painful :) and in case of RichText not as simple as it would seem.
While working with that I discovered that parts of RichText are scattered across ezpublish-kernel (ez\Publish\Core\FieldType\RichText namespace is not everything that makes RichText work).

Also that would mean overriding what already got merged.

However, I managed to split subtree of the mentioned namespace. It could be added as a bunch of separate commits. But because there are parts outside of this namespace, making more splits would be even more time consuming. In some cases (e.g. yaml configuration, Twig helper) it's not possible to make that split at all w/o keeping unrelated code).

This FieldType needs a cleanup (adjustments to having code in a single bundle instead of CoreBundle + Kernel Core - some parts need to go away because of a bundle architecture). So for me it's rather a new code than a modified one.

Maybe the commits where the code is moved from the kernel could have a link to the reference this was splitted from

To summarize: I'm more in favor of this ^ solution.

I'll show you in coming days PR with extracted subtree so you can decide if you prefer it squashed as I do.

@bdunogier

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

The most important is I think that we have traceable history for it. If the commit that adds the files for the first time has links to the point in ezpublish-kernel's history where it was extracted from, I think we are okay.

Maybe it should also include a list of renames that were made ?

One note about the various sources for the files in the kernel: that's exactly why I suggested that you may want to start with ezpublish-kernel, and remove everything that isn't richtext. You end up with a structure that matches what was in the kernel before, but you can use git move to rename the files. Then we keep a full history. Maybe for another time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.