Hi guys

I opened this issue because as Alan and realityloop already know I've
been selected in the Google Summer of Code 2014 with the project 'Porting
the Diff module to Drupal 8'. One of the first steps we (me and my mentor,
Aron Novak) decided that needs to be done is to define the API for Drupal 8
version of Diff module.
I've started to study the implementation of 7.x-3.x-dev and considering the
changes in Drupal 8 we need to define the API for Drupal 8 version of diff
module. Some of the big changes in Field API concerning Diff module are
highlighted below.

Properties in Drupal 7: NID, Title, etc. are now called "base fields"
and are defined in code by the modules defining the entity.
E.g.:
$fields['langcode'] = FieldDefinition::create('language')
->setLabel(t('Language code'))
->setDescription(t('The node language code.'))
->setRevisionable(TRUE);
What used to be called "fields" in Drupal 7 are now "configurable fields"

Do we need to provide to the modules defining the entity the possibility
of adding some other data to the comparison (other than fields) ? or
it is enough now to compare the fields of the entities meaning we don't
need hook_entity_diff anymore since the field comparison is handled by
the diff module only (how this fields are compared is also defined by
the diff module on behalf of the module which defined the field).
If we don't need hook_entity_diff anymore we could get rid of
hook_entity_diff_options() also since we don't need it anymore
(properties are now fields and thus handled by the diff module) and also
all those functions from includes/node.inc and includes/user.inc will be
transformed into MODULE_field_diff_view, MODULE_field_diff_view_prepare, etc.

Considering the above I would kindly ask those of you who know the current
architecture of Diff module and have some knowledge about Drupal 8 to say
if the architecture of the Drupal 8 version of diff can be simplified this way
and also if there are some other architectural changes to be taken into
consideration when defining the API for Drupal 8 version of Diff.

I would really appreciate some advice.

Sandbox link: https://drupal.org/sandbox/lhangea/2269693

Thank you!

CommentFileSizeAuthor
#3 EntityIteratorContents.png115.42 KBlhangea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lhangea’s picture

Issue summary: View changes
Alan D.’s picture

Hi, awesome that you are taking this on.

hook_entity_diff_options()

Happy if hook_entity_diff_options() goes. It is new and not in the latest release (I think). This mainly supports extensions to the functionality so that additional properties can be prepared and rendered, probably not present in any contrib modules.

However, nearly any core property could be of interest in a diff:

Node A and Node B
Node A Revision A and Node A Revision C

So ideas how to manage this are fine. Provide a diff of everything then maybe show core properties in whatever is rendering / building the view.

It is safe to assume a restriction of comparing two entities of the same entity type but potentially different bundles. It is actually possible to compare a node and user now via custom code now, but what would be the point!

MODULE_field_diff_view()

Currently I would see something like this (using the old D7 hooks) if done correctly and generically.

node_entity_diff() - author, created, updated, whatever properties unique to the Node.
user_entity_diff() - created, email, whatever properties unique to the User
field_entity_diff() - any fields attached to the Entity
metatags_entity_diff() - hypothetical differences between different SEO tags
...

This was the direction that I tried to start in D7, never went as far as to totally refactor things to this.

These produce the individual items to compare.

These are then run through diff and the results displayed.

All that said, I do not know about the interface that the core Diff engine has now. Some points to consider:

  • Since we are in alpha, I guess that the old DiffEngine api should be now frozen and should not change much even if it is refactored by sun, et al.
  • Is there a difference in how "base fields" and "configurable fields" are handled internally / rendered?
  • Are some internal properties still outside of the scope of "base fields"? Bundle type comes to mind, although I do not think a bundle change is ever stored (Node Convert module), so this can be ignored if that is the case. I do not know D8 internals yet to comment on this more!
  • Any property / field should be able to be compared directly, not combined into a rendered string prior to the comparison.
  • Potentially move the Diff inline / inline block et al out of the core module? Brian (realityloop)?
  • Potentially move Diff to it's own page to prevent the interaction of other modules altering the revisions page? Or find a way to alter this revisions page without menu overrides? Apparently there is some form of dependency injection or something could be able to do this?? (no idea what I am talking about here)

So if there are things outside of the scope of base / configurable fields, and could potentially change and have these changes stored, then prefixing all of the hooks with field_ is limiting, or invalid naming.

What is the full range of hooks you are considering? Currently you have suggested two view hooks, these suggest rendering of the diff to me, rather than constructing the array of items to diff then render. I think a prototype of what you are intending would provide much more clarity here.

Personally, since there are no upgrade paths to D8, feel free to completely nuke any existing api and build up a clean new one. :)

When I help review, I will look at any potential limitations, but in the end, a functional front end without the internal baggage from D5 is a massive step forward!

lhangea’s picture

FileSize
115.42 KB

First of all thank you for your answer and for the guidelines.

Below I will try to respond to some of the points you've made.

  • Is there a difference in how "base fields" and "configurable fields" are handled internally / rendered?

Basically entity properties and entity fields from Drupal 7 are now called fields.
Properties are now what we find inside a FieldItem object('value', 'format', 'target_id'...).
There are however some differences between base fields and configurable fields (Field API fields):

1. Entity properties from Drupal 7 aren now called base fields and are defined
with the entity type($node->nid, $node->title, $node->uuid etc.)
- Existence establised by code (MyEntityType::baseFieldDefinitions())
- Storage is controlled by the entity storage controller (data stored in some
entity (base) table)
- Better for business logic

2. Configurable fields (aka "Field API" fields)
$node->body, $user->field_custom...
- Defined through an admin UI
- Existence established by config (CMI files)
- Stored in ad-hoc storage tables as in D7
- Managed by the field.module

The idea here is that in Drupal 8 we have a unified API (Entity Field API) on fields which applies to all the fields on an entity (doesn't matter if they are configurable or base fields). I went out and loaded a node entity in Drupal 8
and used Devels dsm. Attached the image with the result to this post. As we can see from that image, every field from the entity is an object which implements FieldItemList or ConfigurableEntityReferenceFieldItemList (which actually extends FieldItemList). My opinion until now is that the big differences between base and config fields are: who establishes they existence and how they are stored.
I think that when it comes to iterating over fields on an entity we can look at them as just fields.

  • Are some internal properties still outside of the scope of "base fields"? ...

I don't think there are internal properties still outside of the scope of entity
fields. See attached image. Are you thinking of something else except what's on
that list?

  • Any property / field should be able to be compared directly...

I'm a little bit confused here.

  • Potentially move Diff to it's own page to prevent the interaction of other modules...

I don't know yet about DI or something like that but what I know is the following way of altering the routes:

/**
 * Listens to the dynamic route events.
 */
class RouteSubscriber extends RouteSubscriberBase {
  /**
   * {@inheritdoc}
   */
  public function alterRoutes(RouteCollection $collection, $provider) {
    // We only respond when routes of the Node module can be altered.
    if ($provider == 'node') {
      $route = $collection->get('node.revision_overview');
      $route->addDefaults(
        array(
          '_content' => '\Drupal\diff\Controller\RevisionController::revisionOverview'
        )
      );
    }
  }
}

The code above alters node.revision_overview route and changes the content displayed on that route with our custom content.

I will try to come up a little bit later with the prototype of what I'm intending.

Alan D.’s picture

Any property / field should be able to be compared directly

I think that you addressed this already with explaining the base fields, :)

altering the routes

This is the weak point in the process, currently Diff and the workflow modules all interact overriding this, leading to, well, to put it nicely, a mess. This does not need to be addressed straightaway, if at all :)

@realityloop
What do you think of a separation of the module to providing a standalone page(s) for the comparison, i.e. to not override the revisions page?

Aron Novak’s picture

Thanks for the very helpful points, Alan!

I did a little research about how to alter the revision overview page without replacing the callback, i think right now it's impossible.

https://drupal.org/node/2138073 - this patch is promising however, it introduces dependency injection for NodeController, but as i see it does not help us.

And that one iss postponed: https://drupal.org/node/1863906 - it would be awesome for us, content revision table as a view, obviously alterable.

I'd say let's go with overriding in the menu system, if the second issue will be completed, we will be able to switch to a sweeter solution very easily. It will help us to start with something very easy.

lhangea’s picture

So, if we restrict the comparison of the entities to entity fields I would say that hook_entity_diff is no longer needed.

Below I list some of the hooks that I think are still needed. Basically this is the field handling part of the diff-7.x.

  • MODULE_field_diff_view()

Callback to the module that defined the field to generate items comparisons.

  • MODULE_field_diff_view_prepare()

Callback to the module that defined the field to prepare items comparison.

  • MODULE_field_diff_options_form()

Callback to the module that defined the field which allows the module to add new specific options to that fields defines by it

  • MODULE_field_diff_default_options()

Callback to the module that defined the field to get the default options for that field

  • hook_field_diff_view_alter()

Allow other modules to interact with MODULE_field_diff_view()

  • hook_field_diff_view_prepare_alter()

Allow other modules to interact with MODULE_field_diff_view_prepare().

  • hook_diff_field_settings_alter()

Allow other modules to alter field settings based on field_context.

lhangea’s picture

Title: GSoC and diff 8.x » Port to Drupal 8
Issue summary: View changes
lhangea’s picture

Issue summary: View changes
Aron Novak’s picture

First, Thanks for the sandbox, it's very nice that we're able to follow.
About the conception in #6, I see two points at first sight:

- the description of the hooks could be a little bit more verbose. MODULE_field_diff_view_prepare() prepares the stuff, ok, but what's the input, what's the output, what may happen inside. So normally someone who is not familiar with the internals of Diff , should be able to understand.

- As we do not have to be backwards compatible, what about restructuring it to have OOP style architecture? https://drupal.org/project/monitoring - this module is a very good example for that. It would be needed to define one or multiple interfaces and that's it. It would be totally in line with the general direction of Drupal 8. I think not a lot has to be changed, just re-designed into interfaces basically.

Alan D.’s picture

When I did have a play months back, I did look at this but my OO skills were both too rusty and not up to date with D8.

But I think that there are two main things to consider if doing this, the Drupal page controllers and then the Diff operations themselves.

If I was to do the porting myself, I would probably consider doing the controllers first, trying to abstract out as much as possible to a base controller. Once this is working, moving onto the core functionality :)

Easy, simple controller for the Node:

class DiffNodeController extends NodeController {}

Hard, a generic controller base accepting a generic comparable entity....

Main consideration is that Entities may or may not have revisions, so two base classes or one with a flag. However, the primary focus should be to get the revision comparisons. But having the base structure for both is very desirable. :)

Diving into the great unknown, so probably totally wrong... something like (two classes has a bad feel personally, so it would be nice to have just one, but maybe two interfaces):

use Drupal\diff\ComparableInterface;

abstract class Drupal\diff\Controller\ComparableController extends ControllerBase {

  public function compare(ComparableInterface $left, ComparableInterface $right) {}

  public function revisionCompare(ComparableInterface $left, ComparableInterface $right) {
    if (!$left->getEntityType()->hasKey('revision') || !$right->getEntityType()->hasKey('revision')) {
      throw new Exception('Revisions are not supported.');
    }
  }
}

class NodeComparableController extends ComparableController {
  // There would probably be nearly nothing in this class :)
}

Throwing a exception within revisionCompare() if a entity type was passed in that doesn't support revisions?

But I am very rusty on all this... too many years doing Drupal, so it is best to get some advice from someone that actually knows the patterns and idioms for this!!!!

lhangea’s picture

@Aron Novak

the description of the hooks could be a little bit more verbose

That's right. If someone wants to get a better understanding of this hooks, some examples of how they work please see diff.api.php (diff-7.x). I'm thinking their functionality won't be changed too much for Drupal 8 (or at least until this moment I couldn't figure out something better).

what about restructuring it to have OOP style

I will be looking into the monitoring project and sure if it's possible for us why not ?

@Alan
Actually I think the idea of using an abstract class which is able to compare entity fields is quite good (as for checking if the entities support revisions I think it's enough if we check if this entities implement RevisionableInterface). Basically by using a base controller we can reproduce the functionality of hook_entity_diff. Every entity implementing RevisionableInterface can extend the base controller and add in there some entity specific methods needed for comparison.

Aron Novak’s picture

The OOP architecture is started to shape form in the sandbox repository:
http://cgit.drupalcode.org/sandbox-lhangea-2269693/tree/src/Controller/E...

I think it has many benefits to do it in Drupal 8 way, as we have the chance to have a clean refactoring now.

Aron Novak’s picture

At https://drupal.org/project/2269693/git-instructions diff_for_drupal_8 , at the master branch, there's the current latest version of the port. There's a shiny new architecture, based on http://www.palantir.net/blog/d8ftw-breadcrumbs-work (very nice finding @lhangea), textfield comparison already works, code reviews and tests from the community are welcome!

Alan D.’s picture

I have not tried to use this nor tried to fully understand this yet, but it is looking slick. Nice work!

I will be excited to test this out when I get time, but current my role is changing from a backend developer to a company directory of a startup having to deal with all of the management process for the first time... so no promises on when I can get a chance to UAT things :)

Alan D.’s picture

Category: Support request » Task
Berdir’s picture

Hi all

Had a quick look at the code, didn't test it yet: I actually did ;)

- diff_form_node_type_edit_form_alter()
You can save this, but you need to do it yourself. See menu_ui_form_node_type_form_alter(), which does something very similar, and you simply need to add your own #submit callback where you can save the values into your configuration.

- Clicking on Global settings for a field gives me an error:
Recoverable fatal error: Argument 1 passed to Drupal\diff\Diff\Entity\FileFieldBuilder::applies() must be an instance of Drupal\Core\Field\FieldDefinitionInterface, array given, called in diff/src/Diff/FieldDiffManager.php on line 137 and defined in Drupal\diff\Diff\Entity\FileFieldBuilder->applies() (line 52 of modules/diff/src/Diff/Entity/FileFieldBuilder.php).

- The following when viewing the revisions tab:
Strict warning: Drupal\Core\Form\FormBase and Drupal\Core\Routing\LinkGeneratorTrait define the same property ($linkGenerator) in the composition of Drupal\diff\Form\RevisionOverviewFor and a number of notices like "Notice: Undefined index: #value in form_pre_render_radio() (line 1045 of core/includes/form.inc)."

- The diff itself seems to be work, just tested title and body field.

- In the settings, I can enable/disable base fields and I can configure settings for field types ( which are currently not visible), not sure where and how I can enable /disable configurable fields or are they always visible?

- The interaction between field types, services that apply to certain field types and the settings forms doesn't make too much sense to me. Is there a reason that you are using tagged services instead of annotated plugins for this? Those have the advantage that you can add additional metadata on them, so you can list them somewhere with a human readable label. I did not look closely at the settings, but wouldn't it make more sense to have settings for those services/plugins instead of field types? A more common pattern for that would then be to use the ConfigurablePluginInterface + PluginFormInterface, so that the settings forms are part of the plugin class that is also using them.

I know we had a similar use case where we needed plugins/something pluggable that applied to certain field types, I'll ask the person who did that to describe that a bit here.

corvus_ch’s picture

I know we had a similar use case where we needed plugins/something pluggable that applied to certain field types, I'll ask the person who did that to describe that a bit here.

That'll be me, I guess.

My use case was the Default Matching Engine of CRM Core. The matching engine allows to identify contacts that are similar to an other based on some configurable criteria. The default matching engine does that by doing queries on a field level. In the D7 version there was a field matcher class for each field type matched by class names. While porting that to D8, I converted them to plugins (actually introduced my own plugin type here). The matching of fields to field matcher is now done by the plugin id. So there is still a one to one mapping between field type and field matcher.

See:

There is one thing I currently want to change. The 1:1 mapping of fields and handlers requires a lot of boilerplate to be written for each field coming along with code duplication. Instead of looking up the handler based on the id, I want add a supports method to the plugin. this allows then to write a single plugin supporting a whole bunch of fields.

This approach was taken in Drupal core for field widgets and field formatters.

Berdir’s picture

Note that widgets/formatters don't use a supports method (which is what the services here are currently) doing, they use applies_to = array(field1, field2) in the annotation.

lhangea’s picture

Hello guys!

First of all thank you for your feedback. Really appreciated.
I will try to address all the issues mentioned in #16.

- diff_form_node_type_edit_form_alter() - solved

- Recoverable fatal error. I need to find a solution to this issue. This issue was introduced when refactoring the array argument of applies method of FieldDiffBuilderInterface to FieldDefinitionInterface.

- Revision tab
linkGenerator warning solved (LinkGeneratorTrait usage was declared in both FormBase and its subclass RevisionOverviewForm class)
Those notices (undefined index) are due to a bug in the Form api in which a radio button in a table row would generate such notices. I created a patch here: https://www.drupal.org/node/2275837 which solves this problem but it is not yet merged.

- Settings for field types can be configured from Diff admin interface -> Global Settings. Base fields visibility can be configured from Entities tab and configurable fields visibility can be set from entity view modes (currently this is not yet implemented).

- Well, what I've tried to achive is a pluggable system in which a user can replace how a field is compared by providing his own tagged service and a proper weight for that service (build method transforms a FieldItemListInterface into a string to be compared and getSettingsForm returns a settings form which exposes some of the field type settings to the end users - Global Settings).
Looking from diff-7.x perspective this approach gave me everything I needed, plus when I started this project I knew little to nothing about plugins in Drupal 8. Anyway I will take a look into plugins, try to understand how they work and of course if it's possible and better this way finally I will refactor it to use plugins instead of a negociated service.

Berdir’s picture

Commented in the #value issue, the patch there is just a bandaid I think that hides the source of the problem, you'd have bigger problem if you would be using more complex form elements.

Plugins are at least as flexible, it's very easy to add an alter hook so that any module can easily replace/change the implementation of a specific plugin and obviously add its own. There is no priority/weight system by default, but you could add that by sorting plugin definitions on a weight key if you'd want that.

lhangea’s picture

All issues from #16 are fixed except for swapping tagged services for plugins.

There is no priority/weight system by default

Well I don't really need a weight system if I use plugins. I used it with tagged services for allowing users to provide their own services instead of the ones defined by the diff module but I suppose with plugins I won't really need it.

lhangea’s picture

Tagged services are now completely removed in favor of plugins (https://github.com/lhangea/Diff/tree/refactor_to_plugins or https://www.drupal.org/sandbox/lhangea/2269693).
Overall I think plugins here are a much better and clean solution.

Thanks again for the suggestions! @Berdir @corvus_ch

Alan D.’s picture

Things appear to be going along nicely :)

Note that using t(), do not wrap the long strings, rather keep these together so that they can be parsed correctly:

- t('Really long line
- with second line');
+ t('Really long line with second line');

And once in the main GIT repo, git.drupal.org:project/diff.git, avoid doing too many individual commits, it is harder to track changes. i.e. 100 commits for the plugins. If this is being done to share code, I would recommend keeping the sandbox repo, then once a feature is complete, merge into the main diff repo. :)

@Brian
Are you happy if I add GIT access? This will assist keeping the developmental history now the main architectural changes are in place. I have added already added reference on the main project page to acknowledge the hard work that Lucian is doing.

Aron Novak’s picture

I could take time for my review as well.

  • Something like diff.api.php is missing, ok, we do not have hooks but we can meaningfully document how to extend diff module
  • At admin/config/content/diff/general, CSS options - Alter the CSS used when displaying diff results., we have no clue where we can add a new variant. Ok, i see that in D7, there were two fixed options and that's all. let's implement it if possible, or remove the option if we only have one. Potentially expose it to others to create new style. (?)
  • At admin/config/content/diff/fields, you can load the plugin settings with the little gear icon, it seems to save its config when you change it, but it doesn't. As views do, we might add a warning message that the config is changed, it's not saved until you save it. I imagine something similar to admin/structure/block .
  • node/1/revisions/view/1/3 - such pages do not have a page title
  • double-check coding standard things, there are a few minor things here and there. example: src/Form/FieldTypesSettingsForm.php array indentations.
  • TextFieldBuilderTest.php is not executable right now
    (Recoverable fatal error: Argument 2 passed to simpletest_run_phpunit_tests() must be of the type array, null given, called in /var/www/html/d8/core/modules/simpletest/simpletest.module on line 143 and defined in simpletest_run_phpunit_tests() (line 192 of /var/www/html/d8/core/modules/simpletest/simpletest.module).)
    do we plan to fix it or is it not really needed? I bet this is something that you mentioned already that with the transition for the plugins, core provides the needed tests.

The core functionality works like a charm after the refactor, i did a test-drive, nice work!

webchick’s picture

Status: Active » Needs review

Marking this as "needs review" since there's already work being done here and that's not obvious from the issue listing. :)

Aron Novak’s picture

It would be quite nice to take an action here, what i saw, the work in GSoC was definitely good to merge it to non-sandbox diff repo and it would give a boost in the further development, i assume.

Alan D.’s picture

Yep, waiting for the others, I do not personally have the rights to grant this.

Aron Novak’s picture

Alan D.: ok, thank you for the feedback at least! Let's try to get follow-up one more time.

miro_dietiker’s picture

I have received commit access to the sandbox by lhangea.
At least we will look into updating the module to stay compatible with current core.

In case there are prerequisites for a merge, please provide further updates in this issue or add a ticket in the sandbox so we can work through them.

jhedstrom’s picture

It makes sense to me to move the sandbox into this module's repo and 8.x branch.

Alan D.’s picture

Added #2419009: Additional D8 Maintainer for request and that should be actioned soon.

Cheers again for the hard work

Anushka-mp’s picture

The module doesn't provide test coverage yet, I've created an issue in the sandbox issue queue and will work on it.

lhangea’s picture

We should probably soon move the development here as here we have more visibility and the issues are not lost .

miro_dietiker’s picture

Status: Needs review » Reviewed & tested by the community

Sure. Please go ahead with promoting it. RTBC then.
And thank you for finding time to making it compatible with latest HEAD again. As of our tests it seems to work today.

I asked Anushka-mp to provide some test coverage since we want to start tracking diff readyness with our Contrib Modules overview / quality assurance.
Berdir started: http://d8ms.worldempire.ch/

As long as the merge didn't happen, i will merge our teams work into your sandbox. Tests will only improve the situation. (thx for passing access.)
Later we are perfectly fine to pass patches here and wait for your review.

Alan D.’s picture

Title: Port to Drupal 8 » [meta] Port to Drupal 8
Status: Reviewed & tested by the community » Needs review

Agreed. Lets move to the main repo to start tracking the history better.

While there have been a number of reviews, I'm not sure that there has been a true comprehensive review to date (I may be wrong), so putting back to Needs review and any issues found can be converted to various sub-tasks and this can serve as a meta issue to track these.

Happy if you tag up an alpha release after merging. This can help get additional testers. If you don't have the rights to do this, I can create a release from the tag for you.

miro_dietiker’s picture

OK here we go.

We didn't do an intense code review. But we have completed a basic test coverage that clicks through all the typical revision management functionality.
#2422735: Provide basic test coverage

Now diff has proper test coverage and we can track its health state. We have no use for the sandbox anymore.
Not setting to RTBC (as i previously did), but i think we should commit this now and publish the branch.

As soon as you did so, ping us through this issue so we can create the 8.x followup issues we identified.

lhangea’s picture

OK, I created the branch 8.x-1.x and pushed it here. Should I create a new release, add tag or how do we proceed from now on ?

miro_dietiker’s picture

U need to create a release node. Otherwise it's not possible to assign tickets to this version.

lhangea’s picture

OK, I created a release from the branch 8.x-1.x.

Alan D.’s picture

I've showed the dev release on the project page. :)

miro_dietiker’s picture

Version: 7.x-3.x-dev » 8.x-1.x-dev

Awesome milestone!

And i created many issues about things i discovered during manual test and basic test coverage review.

Back to work... ;-)

Isn't it time to close this meta port issue now?

lhangea’s picture

Status: Needs review » Closed (fixed)

Yea, I think we can close this now and btw thank you miro for taking time to review the module and for creating those issues.