Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Check Uri for Knowledge Base Creation #400

Merged
merged 1 commit into from
Jul 10, 2017
Merged

Check Uri for Knowledge Base Creation #400

merged 1 commit into from
Jul 10, 2017

Conversation

shinobu
Copy link
Collaborator

@shinobu shinobu commented Oct 4, 2016

This fix checks the URI for knowledge base creation for http://

This Pull Request does fix #399

@shinobu shinobu force-pushed the fix/CreationCheckIsUri branch 2 times, most recently from 39d59ca to d4934ba Compare October 10, 2016 14:56
white-gecko
white-gecko suggested changes Oct 14, 2016
View reviewed changes
Copy link
Member

@white-gecko white-gecko left a comment

Choose a reason for hiding this comment

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

The wording of the error message should be improved. Further it would be good to implement the fix in Erfurt and catch the according exception here.
Please also check at which level OntoWiki is already catching such Exceptions.

application/controllers/ModelController.php Outdated
@@ -511,6 +511,10 @@ public function createAction()
);
$this->view->errorFlag = true;
return;
} else if (!Erfurt_Uri::check($newModelUri)) {
$this->_owApp->appendMessage(
new OntoWiki_Message('The given String is no legal URI (http://...)', OntoWiki_Message::ERROR)
Copy link
Member

Choose a reason for hiding this comment

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

The hint "http://…" connotes, that the algorithm already identified the missing protocol part, while there might be other cases for the value not being a valid URI or IRI.

application/controllers/ModelController.php Outdated
@@ -511,6 +511,10 @@ public function createAction()
);
$this->view->errorFlag = true;
return;
} else if (!Erfurt_Uri::check($newModelUri)) {
$this->_owApp->appendMessage(
new OntoWiki_Message('The given String is no legal URI (http://...)', OntoWiki_Message::ERROR)
Copy link
Member

Choose a reason for hiding this comment

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

"String" is a very technical term in this case, better use "value"

application/controllers/ModelController.php Outdated
@@ -511,6 +511,10 @@ public function createAction()
);
$this->view->errorFlag = true;
return;
} else if (!Erfurt_Uri::check($newModelUri)) {
$this->_owApp->appendMessage(
new OntoWiki_Message('The given String is no legal URI (http://...)', OntoWiki_Message::ERROR)
Copy link
Member

Choose a reason for hiding this comment

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

Since we are aiming for IRI support, it is better to write IRI or URI (resp. other way round)

Copy link
Member

Choose a reason for hiding this comment

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

Also check if the check supports IRIs

@shinobu
Copy link
Collaborator Author

shinobu commented Oct 19, 2016

done in AKSW/Erfurt#129 --- is the error message ok?

@shinobu
Copy link
Collaborator Author

shinobu commented Nov 2, 2016

now uses error codes to check if the exception gets catched or not

@shinobu shinobu mentioned this pull request Nov 13, 2016
Open
@white-gecko
Copy link
Member

Do URNs (urn:…) and https:// URI still work?

@shinobu
Copy link
Collaborator Author

shinobu commented May 2, 2017 •
edited
Loading

are urn:'s really valid RDF/Modelnames?

*edit
this change will make them viable choices as well

@white-gecko
Copy link
Member

white-gecko commented May 3, 2017 •
edited
Loading

What about ftp://… and tel:… I think there are more valid kinds of URIs resp. IRIs and afaik all of them are allowed for models.

https://tools.ietf.org/html/rfc3986
https://tools.ietf.org/html/rfc3987

https://en.wikipedia.org/wiki/Uniform_Resource_Identifier
https://en.wikipedia.org/wiki/Internationalized_Resource_Identifier

@shinobu
Copy link
Collaborator Author

shinobu commented May 12, 2017

We are now using the PHP filter FILTER_VALIDATE_URL, We might need to add a test (the ModelController has no tests at all right now) to check if the filter works for all PHP versions though.

@white-gecko
Copy link
Member

Could you add the test?

@white-gecko white-gecko modified the milestone: 1.1 May 16, 2017
white-gecko
white-gecko suggested changes Jun 20, 2017
View reviewed changes
Copy link
Member

@white-gecko white-gecko left a comment

Choose a reason for hiding this comment

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

Finally please rebase and squash the commits.

application/tests/integration/controller/ModelControllerTest.php Outdated
['urn:oasis:names:specification:docbook:dtd:xml:4.1.2', true],
['https://www.aksw.org/faq', true],
['ptth://www.aksw.org', true],
['\\äüö', false]
Copy link
Member

Choose a reason for hiding this comment

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

Please add some more failing examples

application/tests/integration/controller/ModelControllerTest.php Outdated
public function testCreateActionFiltersOnlyIncorrectUris($uri, $correctUri)
{


Copy link
Member

Choose a reason for hiding this comment

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

Please also check the coding standard with code sniffer

white-gecko
white-gecko suggested changes Jun 28, 2017
View reviewed changes
application/controllers/ModelController.php Outdated
new OntoWiki_Message(
'Given Knowledge Base already exists.',
OntoWiki_Message::ERROR
)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you touch this twice?

application/controllers/ModelController.php
@@ -2,7 +2,7 @@
/**
* This file is part of the {@link http://ontowiki.net OntoWiki} project.
*
* @copyright Copyright (c) 2006-2016, {@link http://aksw.org AKSW}
* @copyright Copyright (c) 2006-2017, {@link http://aksw.org AKSW}
* @license http://opensource.org/licenses/gpl-license.php GNU General Public License (GPL)
Copy link
Member

Choose a reason for hiding this comment

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

This can go to the first commit.

@shinobu
filters incorrect URIs for knowledge base creation out
d28de85 
adds a test for the filter and  adds  frontendlogin into the ControllerTestCase.php
@shinobu
Copy link
Collaborator Author

shinobu commented Jul 8, 2017

Done @white-gecko
This should be mergeable now

@white-gecko white-gecko merged commit 9c20df6 into develop Jul 10, 2017
@white-gecko white-gecko deleted the fix/CreationCheckIsUri branch July 10, 2017 09:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@white-gecko white-gecko white-gecko approved these changes

Assignees

@shinobu shinobu

Labels
None yet
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

Using a non uri as uri for creating a knowledge base leads to a fatal error
3 participants
@shinobu @white-gecko @pfrischmuth