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!
Comment | File | Size | Author |
---|---|---|---|
#3 | EntityIteratorContents.png | 115.42 KB | lhangea |
Comments
Comment #1
lhangea CreditAttribution: lhangea commentedComment #2
Alan D. CreditAttribution: Alan D. commentedHi, 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:
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!
Comment #3
lhangea CreditAttribution: lhangea commentedFirst 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.
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.
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?
I'm a little bit confused here.
I don't know yet about DI or something like that but what I know is the following way of altering the routes:
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.
Comment #4
Alan D. CreditAttribution: Alan D. commentedI think that you addressed this already with explaining the base fields, :)
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?
Comment #5
Aron NovakThanks 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.
Comment #6
lhangea CreditAttribution: lhangea commentedSo, 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.
Callback to the module that defined the field to generate items comparisons.
Callback to the module that defined the field to prepare items comparison.
Callback to the module that defined the field which allows the module to add new specific options to that fields defines by it
Callback to the module that defined the field to get the default options for that field
Allow other modules to interact with MODULE_field_diff_view()
Allow other modules to interact with MODULE_field_diff_view_prepare().
Allow other modules to alter field settings based on field_context.
Comment #7
lhangea CreditAttribution: lhangea commentedSandbox link: https://drupal.org/sandbox/lhangea/2269693
Comment #8
lhangea CreditAttribution: lhangea commentedComment #9
Aron NovakFirst, 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.
Comment #10
Alan D. CreditAttribution: Alan D. commentedWhen 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:
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):
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!!!!
Comment #11
lhangea CreditAttribution: lhangea commented@Aron Novak
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).
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.
Comment #12
Aron NovakThe 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.
Comment #13
Aron NovakAt 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!
Comment #14
Alan D. CreditAttribution: Alan D. commentedI 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 :)
Comment #15
Alan D. CreditAttribution: Alan D. commentedComment #16
BerdirHi 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.
Comment #17
corvus_ch CreditAttribution: corvus_ch commentedThat'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.
Comment #18
BerdirNote 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.
Comment #19
lhangea CreditAttribution: lhangea commentedHello 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.
Comment #20
BerdirCommented 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.
Comment #21
lhangea CreditAttribution: lhangea commentedAll issues from #16 are fixed except for swapping tagged services for plugins.
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.
Comment #22
lhangea CreditAttribution: lhangea commentedTagged 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
Comment #23
Alan D. CreditAttribution: Alan D. commentedThings 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:
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.
Comment #24
Aron NovakI could take time for my review as well.
(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!
Comment #25
webchickMarking this as "needs review" since there's already work being done here and that's not obvious from the issue listing. :)
Comment #26
Aron NovakIt 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.
Comment #27
Alan D. CreditAttribution: Alan D. commentedYep, waiting for the others, I do not personally have the rights to grant this.
Comment #28
Aron NovakAlan D.: ok, thank you for the feedback at least! Let's try to get follow-up one more time.
Comment #29
miro_dietikerI 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.
Comment #30
jhedstromIt makes sense to me to move the sandbox into this module's repo and 8.x branch.
Comment #31
Alan D. CreditAttribution: Alan D. commentedAdded #2419009: Additional D8 Maintainer for request and that should be actioned soon.
Cheers again for the hard work
Comment #32
Anushka-mp CreditAttribution: Anushka-mp commentedThe module doesn't provide test coverage yet, I've created an issue in the sandbox issue queue and will work on it.
Comment #33
lhangea CreditAttribution: lhangea commentedWe should probably soon move the development here as here we have more visibility and the issues are not lost .
Comment #34
miro_dietikerSure. 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.
Comment #35
Alan D. CreditAttribution: Alan D. commentedAgreed. 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.
Comment #36
miro_dietikerOK 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.
Comment #37
lhangea CreditAttribution: lhangea commentedOK, 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 ?
Comment #38
miro_dietikerU need to create a release node. Otherwise it's not possible to assign tickets to this version.
Comment #39
lhangea CreditAttribution: lhangea commentedOK, I created a release from the branch 8.x-1.x.
Comment #40
Alan D. CreditAttribution: Alan D. commentedI've showed the dev release on the project page. :)
Comment #41
miro_dietikerAwesome 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?
Comment #42
lhangea CreditAttribution: lhangea commentedYea, I think we can close this now and btw thank you miro for taking time to review the module and for creating those issues.