Graph refactoring #13

claudiu-cristea · 2018-01-09T19:57:30Z

Working on this refactor, I've also opened next issues: #14, #15, #16, #17, #18, #19.

…g entities.

…ule gets enabled.

idimopoulos · 2018-01-19T12:44:26Z

API.md

+
+Entities using the SPARQL storage can be stored in different graphs, which are
+actually versions of the same entity, You can use graphs, for example, to store
+a draft version of the entity.


In reality, this is a Joinup feature and it is going against the philosophy of the RDF. A graph is a group of triples that are supposed to describe entities belonging to a certain thematic group, e.g. collections, movies, authors etc.

This small piece could refer to the rdf draft module that uses the graphs as states to store versions of the same entity.

So graphs are in fact bundles (entities belonging to a certain thematic group, e.g. collections, movies, authors etc.) ? :)

And the "draft" thing was only to give an example. Of course, based on our use case.

idimopoulos · 2018-01-19T12:46:48Z

API.md

+
+### Graphs CRUD
+
+Graphs are config entities of type `rdf_entity_graph` and are supporting the


did you mean 'are supported by'?

I said "are supporting". That means they are subject of the whole config entity API.

idimopoulos · 2018-01-19T12:51:35Z

API.md

+
+// Load from the first graph where the entity exists. First, the storage will
+// attempt to load the entity from the 'draft' graph. If this entity doesn't
+// exist in the 'draft' graph, will fallback to the next one which is 'sync' and


Marking to be commented: I haven't looked into all the changes yet, but last time I checked, all triples were returned from the database from all available graphs. The triples were the processed by the RdfEntitySparqlStorage class and the appropriate entity was returned instead.

To be checked later on.

he triples were the processed by the RdfEntitySparqlStorage

Yes. this ($storage) is actually a RdfEntitySparqlStorage class. So, it returns a single entity, not all.

idimopoulos · 2018-01-19T12:53:02Z

API.md

+// Load from the first graph where the entity exists. First, the storage will
+// attempt to load the entity from the 'draft' graph. If this entity doesn't
+// exist in the 'draft' graph, will fallback to the next one which is 'sync' and
+// so on. If fails with all, the normal behaviour is in place: will return NULL.


'If the entity is not found in any graph' or 'If searching in all graphs fail' is more appropriate here.

OK. Changed.

idimopoulos · 2018-01-19T12:58:04Z

modules/rdf_draft/rdf_draft.info.yml

 core: 8.x
-package: Joinup
+package: Custom


Maybe we could have our own name since all of these modules are related to RDF data. 'RDF Entity' could do for now.

You're right but I expanded too much the initial scope of this issue. Opened #14 to do this change.

idimopoulos · 2018-01-19T14:37:40Z

rdf_entity.module

+  $mapping = $form_state->get('rdf_entity_mapping');
+  $values = $form_state->getValue('rdf_entity');
+
+  try {


Why try? Normally we should have a stable version. Try won't do any good here as it is only 'debugging' it.

If something breaks we should allow it to break because it means we have a bug in our code.

OK, let's propagate the exception but I still want to log it.

idimopoulos · 2018-01-19T14:51:01Z

src/Controller/RdfEntityGraphToggle.php

+use Drupal\rdf_entity\RdfEntityGraphInterface;
+
+/**
+ * Toggles a RDf entity graph to enable or disable.


'Toggles an' is correct here. The 'a' or 'an' follows the pronunciation of the next word and in the case of an acronym, the sound is taken into an account. In the current example, it's read as 'Ar Di Ef' so it is 'an Ar Di Ef entity'.

Disclaimer: I am not a language expert, it just happened that we had the exact same issue for the 'RDF' word and we looked it up :)

idimopoulos · 2018-01-19T16:20:08Z

src/Entity/RdfEntityGraph.php

+ * Used to store basic information about each RDF entity graph.
+ *
+ * @ConfigEntityType(
+ *   id = "rdf_entity_graph",


This can probably be rdf_graph.

No. I like (and it's a good practice) to fully namespace the identifiers. So "rdf_entity" is the module name and "graph" the entity type itself. The same I did for rdf_entity_mapping.

idimopoulos · 2018-01-19T21:39:49Z

rdf_entity.module

@@ -29,6 +36,8 @@ function rdf_entity_entity_base_field_info_alter(&$fields, EntityTypeInterface $
    return;
  }

+  // @todo Now that graphs are entities, transform this field into an entity


I checked all the bundles and this properties is now empty. Isn't this replaced by the graph entities?

I checked all the bundles and this properties is now empty. Isn't this replaced by the graph entities?

Not sure I understand your question. This is a base field added to all rdf_entity entities. The field is a URI (basically a string) and it's stored in the 'value' column. Now that the graphs are config entities we could transform this field into a entity reference because now it stores the ID of an rdf_entity_graph entity. I've opened a follow-up in #15.

Oh $&#* I totally understood wrong here.. I got confused by the label of the field. Sorry. This is wrong by me.

idimopoulos · 2018-01-22T08:52:35Z

src/RdfGraphHandler.php

   */
-  public function __construct(EntityTypeManagerInterface $entity_type_manager, ModuleHandlerInterface $module_handler) {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager) {


You can always initiate the $rdfEntityGraphStorage in the constructor as well.

I prefer the lazy-loading method

…g_property().

sandervd · 2018-01-22T15:38:53Z

modules/rdf_draft/rdf_draft.module

+    $storage = \Drupal::entityTypeManager()->getStorage($entity->getEntityTypeId());
+  }
+  catch (\Exception $exception) {
+    return [];


Is masking the exception intended here?

sandervd · 2018-01-22T15:44:53Z

rdf_entity.install

+  // RdfEntitySparqlStorage and move their 3rd party settings belonging to
+  // rdf_entity module into their dedicated rdf_entity_mapping config entities.
+  foreach ($entity_type_manager->getDefinitions() as $entity_type_id => $entity_type) {
+    if ($storage = $entity_type_manager->getStorage($entity_type_id)) {


Maybe it is just esthetic, but the cyclomatic complexity could be decreased here by inverting the check and issuing a continue; after each check. e.g.
if ( !($storage instanceof RdfEntitySparqlStorage) ) {
continue;
}

sandervd · 2018-01-22T15:48:46Z

rdf_entity.module

-    $graph_uri = $form_state->getValue('graph_' . $definedGraphName);
-    $third_party_settings[$definedGraphName] = $graph_uri;
+  catch (\Exception $exception) {
+    \Drupal::logger('rdf_entity')->critical(t('Could not save RDF entity mapping'), ['exception' => $exception]);


I think all non-caught runtime exceptions are logged anyway...

sandervd · 2018-01-22T15:52:05Z

src/Entity/RdfEntityMapping.php

+          }
+        }
+        catch (\Exception $exception) {
+          // Fail silently to allow the next graphs to be added.


It looks like we're using exceptions here for flow control.

sandervd · 2018-01-22T16:00:32Z

src/Entity/RdfEntitySparqlStorage.php

   *
   * @throws \Exception
   *    Thrown when the entity graph is empty.
   */
-  protected function processGraphResults($results) {
+  protected function processGraphResults($results, array $graph_ids): ?array {


Would be great if this could get some 'extract method' love...

sandervd · 2018-01-23T07:44:13Z

modules/rdf_draft/tests/src/Kernel/RdfDraftGraphTest.php

@@ -0,0 +1,114 @@
+<?php


❤️ ❤️ ❤️ ❤️ ❤️ ❤️

claudiu-cristea

ops

claudiu-cristea

aaa

claudiu-cristea · 2018-01-23T12:12:29Z

modules/rdf_draft/rdf_draft.module

+    $storage = \Drupal::entityTypeManager()->getStorage($entity->getEntityTypeId());
+  }
+  catch (\Exception $exception) {
+    return [];


Ok. Let's unleash the daemons!

claudiu-cristea · 2018-01-23T12:19:33Z

rdf_entity.install

+  // RdfEntitySparqlStorage and move their 3rd party settings belonging to
+  // rdf_entity module into their dedicated rdf_entity_mapping config entities.
+  foreach ($entity_type_manager->getDefinitions() as $entity_type_id => $entity_type) {
+    if ($storage = $entity_type_manager->getStorage($entity_type_id)) {


+1 for this. Also if ($storage = .... ->getStorage(...)) {...} doesn't make any sense because ::getStorage() doesn't return anything. It only throws an exception if fails the return the storage.

claudiu-cristea · 2018-01-23T12:23:13Z

rdf_entity.module

-    $graph_uri = $form_state->getValue('graph_' . $definedGraphName);
-    $third_party_settings[$definedGraphName] = $graph_uri;
+  catch (\Exception $exception) {
+    \Drupal::logger('rdf_entity')->critical(t('Could not save RDF entity mapping'), ['exception' => $exception]);


Never thought. But, yes, it makes sense. Let's simplify then.

claudiu-cristea · 2018-01-23T12:26:02Z

src/Entity/RdfEntityMapping.php

+          }
+        }
+        catch (\Exception $exception) {
+          // Fail silently to allow the next graphs to be added.


It's more to satisfy the IDE. Let's drop that too.

claudiu-cristea · 2018-01-23T12:52:23Z

src/Entity/RdfEntitySparqlStorage.php

   *
   * @throws \Exception
   *    Thrown when the entity graph is empty.
   */
-  protected function processGraphResults($results) {
+  protected function processGraphResults($results, array $graph_ids): ?array {


I don't get it. Could you explain?

Discussed with @sandervd to move this change in #19 followup.

idimopoulos · 2018-01-23T14:38:04Z

Merging :) Good job :)

claudiu-cristea added 6 commits January 9, 2018 14:55

RDF Entity module requires PHP >=7.1.

d088448

Fix docs in src/RdfGraphHandler.

5a05af6

Add schema for 'rdf_entity_graph' and 'rdf_entity_mapping' entities.

e404205

Move mapping config from bundle 3rd party settings to dedicated confi…

3532abe

…g entities.

Use the mapping stored in the dedicated config entity.

05be3d9

Add the rdf_entity_graph config entity.

a946220

claudiu-cristea force-pushed the graph-refactor branch from 586fbca to a946220 Compare January 9, 2018 19:58

Additional checks for field existence in base fields mapping.

1ea6809

claudiu-cristea force-pushed the graph-refactor branch from 67858de to a347ddb Compare January 10, 2018 09:55

claudiu-cristea added 3 commits January 10, 2018 12:03

Update path: move mapping configs in their dedicated storage.

edfdde7

Add an interface to RdfEntityGraph class.

d5dc9b1

Expand the RdfEntityMapping with setters and getters.

4d703dc

claudiu-cristea force-pushed the graph-refactor branch from a347ddb to 4d703dc Compare January 10, 2018 10:03

claudiu-cristea added 7 commits January 10, 2018 16:31

Fix the return type.

28ec21f

YAML standards on rdf_draft.info.yml.

d30b89d

::getGraph() was renamed to ::getGraphUri().

885d3e4

Add setters/getters and a new property 'entity_types' to RdfEntityGraph.

ae40916

Refactor \Drupal\rdf_entity\RdfGraphHandler::getGraphDefinitions().

5b62742

Add the 'draft' graph.

7fa3c20

Change the label of 'default' graph to 'Published' when rdf_draft mod…

d19927b

…ule gets enabled.

claudiu-cristea force-pushed the graph-refactor branch from e8be6ff to c8aff0a Compare January 18, 2018 17:55

Completely modernize RdfGraphHandler & RdfEntitySparqlStorage

2f7a793

claudiu-cristea force-pushed the graph-refactor branch from c8aff0a to 2f7a793 Compare January 18, 2018 17:58

Additional fixes.

5181d63

idimopoulos suggested changes Jan 22, 2018

View reviewed changes

claudiu-cristea added 5 commits January 22, 2018 17:24

Add reference to #15 follow-up.

39f4366

Rename rdf_entity_get_third_party_property() to rdf_entity_get_mappin…

4d00706

…g_property().

Allow the exception to propagate when saving a mapping in the UI.

4f369a0

Remove stale @todo.

322d6d8

Throw exception when invalid graphs are passed to storage loaders.

ccc59d8

claudiu-cristea added 2 commits January 22, 2018 17:30

Missing strict type declaration.

0ae10cd

Typo in RdfEntityGraphToggle docs.

0d15115

claudiu-cristea force-pushed the graph-refactor branch from d348dee to 75f523e Compare January 22, 2018 16:58

Improve wording in API.md.

11b92c7

claudiu-cristea force-pushed the graph-refactor branch from 75f523e to 11b92c7 Compare January 22, 2018 16:59

claudiu-cristea mentioned this pull request Jan 22, 2018

Support entity types without bundles or without bundle config entities #18

Closed

sandervd reviewed Jan 23, 2018

View reviewed changes

claudiu-cristea commented Jan 23, 2018

View reviewed changes

claudiu-cristea added 2 commits January 23, 2018 14:49

Use the RdfGraphHandlerInterface instead of class on type hinting.

de7fb03

Unmute some exceptions.

31828f7

claudiu-cristea commented Jan 23, 2018

View reviewed changes

Add followups to some @todos.

477f582

idimopoulos approved these changes Jan 23, 2018

View reviewed changes

idimopoulos merged commit cf357d4 into 8.x-1.x Jan 23, 2018

idimopoulos deleted the graph-refactor branch January 23, 2018 14:59


		### Graphs CRUD

		Graphs are config entities of type `rdf_entity_graph` and are supporting the

Graph refactoring #13

Graph refactoring #13

Conversation

claudiu-cristea commented Jan 9, 2018 • 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

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

claudiu-cristea Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiu-cristea Jan 22, 2018 • 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

claudiu-cristea left a comment

Choose a reason for hiding this comment

claudiu-cristea 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

idimopoulos commented Jan 23, 2018

claudiu-cristea commented Jan 9, 2018 •

edited

Loading

claudiu-cristea Jan 22, 2018 •

edited

Loading

claudiu-cristea Jan 22, 2018 •

edited

Loading