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

Jump to bottom

Use EasyRDF in sfSkosPlugin, refs #10621 #474

Merged

qubot merged 3 commits into qa/2.4.x from dev/issue-10621-skos-easyrdf

Dec 20, 2016

Member

sevein commented Nov 29, 2016

Redmine: https://projects.artefactual.com/issues/10621

sevein added the work-in-progress label

sevein added this to the qa/2.4.x milestone

qubot force-pushed the dev/issue-10621-skos-easyrdf branch 3 times, most recently from 5b139dc to 29344b1 Compare

December 6, 2016 00:04

sevein removed the work-in-progress label

qubot force-pushed the dev/issue-10621-skos-easyrdf branch 8 times, most recently from 36a19e0 to c80849c Compare

December 7, 2016 06:51

qubot force-pushed the dev/issue-10621-skos-easyrdf branch 5 times, most recently from d1d4cd1 to 56d3b08 Compare

December 19, 2016 23:34

sevein requested a review from jraddaoui

December 20, 2016 00:43

qubot force-pushed the dev/issue-10621-skos-easyrdf branch from 56d3b08 to d5eeda0 Compare

December 20, 2016 06:04

jraddaoui reviewed

View reviewed changes

Contributor

jraddaoui left a comment

Impressive! I've added a few minor comments but it looks great overall.

lib/model/QubitJob.php

    
                  if (!isset($this->statusId))

                  {

                    return 'unknown';

                    return $unknown;

                  }

Contributor

jraddaoui Dec 20, 2016

Not a big deal and I'm not completely sure, but I think that if the statusId is not set it will fall in the switch default case.

Member Author

sevein Dec 20, 2016

You're right! I guess this is the result of having switched to Python for a while now, where AttributeError would be raised if you try to access to a member of an object that is not defined. I'll fix it!

lib/job/arJobLogger.class.php

+                  if (!isset($this->options['job']))
+                  {
+                    throw new sfException('Missing parameter job');

Contributor

jraddaoui Dec 20, 2016

I'm not a native but I think "Missing job parameter" sounds better.

Contributor

fiver-watson Dec 20, 2016

Agree! :D

Member Author

sevein Dec 20, 2016

Hehehe thank you!

plugins/sfSkosPlugin/lib/sfSkosPlugin.class.php

@@ @@ -17,359 +17,315 @@ @@
                * along with Access to Memory (AtoM).  If not, see <http://www.gnu.org/licenses/>.
                */
+              class sfSkosPluginException extends Exception {}

Contributor

jraddaoui Dec 20, 2016

I'd expect to find this in a different file, like QubitApiException.class.php

plugins/sfSkosPlugin/lib/sfSkosPlugin.class.php

    
                  // Get all concepts

                  $concepts = $skos->xpath->query("skos:Concept | $rdfsel");

                  $this->i18n = sfContext::getInstance()->i18n;

                  $this->graph = new EasyRdf_Graph;

Contributor

jraddaoui Dec 20, 2016

We could initiate $this->graph after the checks bellow

plugins/sfSkosPlugin/lib/sfSkosPlugin.class.php

+                  $this->setScopeNote($term, $concept);
+                  $this->setUriSourceNote($term, $concept);
+                  // Map dc.coverage to term.code for place terms. Hacky Hackerton was here.

Contributor

jraddaoui Dec 20, 2016

😄

Member Author

sevein Dec 20, 2016

I actually didn't write that! I think it was Jack!

plugins/sfSkosPlugin/lib/sfSkosPlugin.class.php

+                    $otherName = new QubitOtherName;
+                    $otherName->typeId = QubitTerm::ALTERNATIVE_LABEL_ID;
+                    $otherName->sourceCulture = $item->getLang();
+                    $otherName->__set('name', $item->getValue(), array('culture' => $lang));

Contributor

jraddaoui Dec 20, 2016

No need to change it, but I think you can use $otherName->setName($item->getValue(), array('culture' => $lang)); for a little more magic in cases like this

plugins/sfSkosPlugin/lib/sfSkosPlugin.class.php

-                      {
-                        continue 2;
-                      }
+                      $relations->insert($c->get('atom:id')->getValue(), $r->get('atom:id')->getValue());

Contributor

jraddaoui Dec 20, 2016

Love it!

plugins/sfSkosPlugin/lib/sfSkosPlugin.class.php

+                  // Re-index parent term so numberOfDescendants reflects the changes
+                  if (QubitTerm::ROOT_ID != $this->parent->id)
+                  {
+                    QubitSearch::getInstance()->update($this->parent);

Contributor

jraddaoui Dec 20, 2016

Not a big deal, as the terms are not so big in the ES index, but we could use partial updates in here.

Member Author

sevein Dec 20, 2016 •

edited

That'd be cool but where would you take the new value from? It'd be weird to instantiate a arElasticSearchTermPdo only for that reason. If I make the calculation then I'm duplicating code. If I use the ORM I'd need to hydrate the object again I believe?
It'd be very nice if the partialUpdate method was able to generate the new value for me in case I don't give him once...

plugins/sfSkosPlugin/modules/sfSkosPlugin/actions/importAction.class.php

+                        '%1%' => sprintf('<a href="%s">', $this->context->routing->generate(null, array('module' => 'jobs', 'action' => 'report', 'id' => $job->id))),
+                        '%2%' => $job->id,
+                        '%3%' => '</a>'
+                      )), array('persist' => false));

Contributor

jraddaoui Dec 20, 2016

Could you check the persist option in the other flash alerts we set? e.g.: https://github.com/artefactual/atom/blob/qa/2.4.x/apps/qubit/modules/informationobject/actions/updatePublicationStatusAction.class.php#L106

Member Author

sevein Dec 20, 2016

On that particular example I think it's good that it's persisted so it survives to the redirection, right? In any case, there is a total of 16 uses of setFlash... it sounds like we should do that on a separate pull request?

Contributor

jraddaoui Dec 20, 2016

Sounds good!

plugins/sfSkosPlugin/test/unit/uniqueTest.php

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

Contributor

jraddaoui Dec 20, 2016

I'd name it 'uniqueRelationsTest.php'

qubot force-pushed the dev/issue-10621-skos-easyrdf branch 2 times, most recently from 206ba4c to 7acf7af Compare

December 20, 2016 18:14

Contributor

jraddaoui commented Dec 20, 2016

👍

sevein added 3 commits

December 20, 2016 18:40


          Small clean-ups, refs #10621

a145b81

These are the changes in this commit:

- i18n in Job status
- Link to Job report after import in object/ module
- CSS tweaks
- Stop using `$this` in Qubit.class.php


          New custom logger for jobs, refs #10621

A new logger called arJobLogger is created by arBaseJob with context about the
current running job. This new logger has been created so it can be injected to
other parts of the application where a logger is accepted so we can capture
their events.

arJobLogger also dispatches logs to `gearman.worker.log` as there is an
observer in `jobWorkerTask.class.php` passing these messages to sfTask so
ultimately they're shown in the standard stream.


          sfSkosPlugin: refactor using EasyRdf, refs #10621

5ed896b

This commit vendors EasyRDF.

qubot force-pushed the dev/issue-10621-skos-easyrdf branch from 7acf7af to 5ed896b Compare

December 20, 2016 18:41

qubot merged commit 5ed896b into qa/2.4.x

qubot deleted the dev/issue-10621-skos-easyrdf branch

December 20, 2016 20:14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment