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

Address https://github.com/Islandora-CLAW/CLAW/issues/302 ; Query Par… #51

Merged
merged 12 commits into from
Jul 22, 2016

Conversation

ruebot
Copy link
Member

@ruebot ruebot commented Jun 30, 2016

…ameters: CHECKSUM (Optional - DEPRECATED)

See: Islandora/documentation#302

@whikloj
Copy link
Member

whikloj commented Jul 4, 2016

So this changes how the checksum is formatted. We used to just pass a checksum, but now that checksum is actually "<checksum type>=<checksum>".

For example, in the Chullo README there is an example of

$child_uri = $chullo->createResource(
        $uri,
        $rdf,
        ['Content-Type' => 'text/turtle'],
        $transaction,
        sha1($rdf)
    );

That now fails due to the change in Fedora, but using this change also doesn't work.

We need to change it to specify the checksum type as well as the checksum.

Like

$child_uri = $chullo->createResource(
        $uri,
        $rdf,
        ['Content-Type' => 'text/turtle'],
        $transaction,
        "sha1=" . sha1($rdf)
    );

So the code is fine, but the argument is changing.

I just want to make sure that we are okay with this, or should we separate the information out into two arguments or an array of arguments? <shrug>

@ruebot
Copy link
Member Author

ruebot commented Jul 4, 2016

I just want to make sure that we are okay with this, or should we separate the information out into two arguments or an array of arguments?

We'll, since fcrepo4 really only supports one fixity type right now, I'm fine with leaving it like that. Once we get more, then we can add more.

...but...hrm...what about migrating fcrepo3 fixity information?

@whikloj
Copy link
Member

whikloj commented Jul 4, 2016

I haven't tried (but I will) using a "md5=blah" checksum

@whikloj
Copy link
Member

whikloj commented Jul 4, 2016

Ok, so md5 seems to be disregarded.
I did
$new_image = $api->createResource("", $image_content, $headers, "", 'md5=823478435');
and it created the resource but
$new_image = $api->createResource("", $image_content, $headers, "", 'sha1=823478435');
gave a 409 Conflict response.
FYI, I made up the checksum.

@ruebot
Copy link
Member Author

ruebot commented Jul 4, 2016

@whikloj should we change it from a string to an array? Or have two string values, checksum_algorithm and checksum_value?

@ruebot
Copy link
Member Author

ruebot commented Jul 4, 2016

@whikloj if that's the route we want to go, I can work on updating the test where it failed.

@codecov-io
Copy link

codecov-io commented Jul 21, 2016

Current coverage is 86.48% (diff: 100%)

Merging #51 into master will increase coverage by 0.58%

@@             master        #51   diff @@
==========================================
  Files             4          4          
  Lines           227        222     -5   
  Methods          36         36          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            195        192     -3   
+ Misses           32         30     -2   
  Partials          0          0          

Powered by Codecov. Last update 1593cae...9abaa57

@@ -226,21 +232,23 @@ public function saveResource(
public function saveGraph(
$uri,
\EasyRdf_Graph $graph,
$checksum_algorithm = "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this line.

@DiegoPino
Copy link
Contributor

@ruebot that last one looks fine. $headers is the way, less arguments, closer to fcrepo REST API 👍

@ruebot
Copy link
Member Author

ruebot commented Jul 21, 2016

@whikloj @DiegoPino I still need to update Crayfish and PDX, and then this issue should be resolved.

@ruebot
Copy link
Member Author

ruebot commented Jul 21, 2016

Islandora/Crayfish#12

@ruebot
Copy link
Member Author

ruebot commented Jul 21, 2016

Nothing in PDX according to grep.

@whikloj
Copy link
Member

whikloj commented Jul 22, 2016

Error in test is because you need to define the base_uri on the Client here. Check the other test.

@whikloj whikloj merged commit 89a50b3 into Islandora:master Jul 22, 2016
@ruebot ruebot deleted the issue-302 branch July 22, 2016 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants