fixes undefined offset by adding an isset test

[kouralex]

Whilst checking php error log for a Skosmos instance I noticed that the line 467 below

Skosmos/controller/WebController.php

Lines 443 to 473 in 254c2c6

	/**
	* Invokes the alphabetical listing for a specific vocabulary.
	*/
	public function invokeAlphabeticalIndex($request)
	{
	$lang = $request->getLang();
	$this->setLanguageProperties($lang);
	$template = $this->twig->loadTemplate('alphabetical-index.twig');
	$vocab = $request->getVocab();
	$offset = ($request->getQueryParam('offset') && is_numeric($request->getQueryParam('offset')) && $request->getQueryParam('offset') >= 0) ? $request->getQueryParam('offset') : 0;
	if ($request->getQueryParam('limit')) {
	$count = $request->getQueryParam('limit');
	} else {
	$count = ($offset > 0) ? null : 250;
	}
	$contentLang = $request->getContentLang();
	$allAtOnce = $vocab->getConfig()->getAlphabeticalFull();
	if (!$allAtOnce) {
	$letters = $vocab->getAlphabet($contentLang);
	$letter = $request->getLetter();
	if ($letter === '') {
	$letter = $letters[0];
	}
	$searchResults = $vocab->searchConceptsAlphabetical($letter, $count, $offset, $contentLang);
	} else {
	$letters = null;
	$searchResults = $vocab->searchConceptsAlphabetical('*', null, null, $contentLang);
	}

can cause Undefined offset: 0 PHP Notice.

By following the code it calls it seems like the NULL value is then passed here:

Skosmos/model/sparql/GenericSparql.php

Lines 1187 to 1216 in 254c2c6

	/**
	* Generates sparql query clauses used for creating the alphabetical index.
	* @param string $letter the letter (or special class) to search for
	* @return array of sparql query clause strings
	*/
	private function formatFilterConditions($letter, $lang) {
	$useRegex = false;
	if ($letter == '*') {
	$letter = '.*';
	$useRegex = true;
	} elseif ($letter == '0-9') {
	$letter = '[0-9].*';
	$useRegex = true;
	} elseif ($letter == '!*') {
	$letter = '[^\\\\p{L}\\\\p{N}].*';
	$useRegex = true;
	}
	# make text query clause
	$lcletter = mb_strtolower($letter, 'UTF-8'); // convert to lower case, UTF-8 safe
	if ($useRegex) {
	$filtercondLabel = $lang ? "regex(str(?label), '^$letter$', 'i') && langMatches(lang(?label), '$lang')" : "regex(str(?label), '^$letter$', 'i')";
	$filtercondALabel = $lang ? "regex(str(?alabel), '^$letter$', 'i') && langMatches(lang(?alabel), '$lang')" : "regex(str(?alabel), '^$letter$', 'i')";
	} else {
	$filtercondLabel = $lang ? "strstarts(lcase(str(?label)), '$lcletter') && langMatches(lang(?label), '$lang')" : "strstarts(lcase(str(?label)), '$lcletter')";
	$filtercondALabel = $lang ? "strstarts(lcase(str(?alabel)), '$lcletter') && langMatches(lang(?alabel), '$lang')" : "strstarts(lcase(str(?alabel)), '$lcletter')";
	}
	return array('filterpref' => $filtercondLabel, 'filteralt' => $filtercondALabel);
	}

where at line 1207 it is again (luckily) changed back to an empty string. This will, however, cause a filter clause that has a STRSTARTS() with an empty argument, which is unnecessary and will always evaluate to True.

Whilst this PR fixes the PHP notice, it won't change the behavior (i.e., the query as it is, now). I wonder if it would be more appropriate to, in this case, change the letter value to "*", for which there is a special treatment (unlike for empty string). However, now that I look into that, it seems like the STRSTARTS part is unnecessary for the aforementioned "*" value.

Additionally, I just noticed that this also affects the JenaTextSparql:

Skosmos/model/sparql/JenaTextSparql.php

Lines 102 to 171 in 254c2c6

	/**
	* Generates the jena-text-specific sparql query used for rendering the alphabetical index.
	* @param string $letter the letter (or special class) to search for
	* @param string $lang language of labels
	* @param integer $limit limits the amount of results
	* @param integer $offset offsets the result set
	* @param array|null $classes
	* @param boolean $showDeprecated whether to include deprecated concepts in the result (default: false)
	* @param \EasyRdf\Resource|null $qualifier alphabetical list qualifier resource or null (default: null)
	* @return string sparql query
	*/
	public function generateAlphabeticalListQuery($letter, $lang, $limit = null, $offset = null, $classes = null, $showDeprecated = false, $qualifier = null)
	{
	if ($letter == '*' || $letter == '0-9' || $letter == '!*') {
	// text index cannot support special character queries, use the generic implementation for these
	return parent::generateAlphabeticalListQuery($letter, $lang, $limit, $offset, $classes, $showDeprecated, $qualifier);
	}
	$gc = $this->graphClause;
	$classes = ($classes) ? $classes : array('http://www.w3.org/2004/02/skos/core#Concept');
	$values = $this->formatValues('?type', $classes, 'uri');
	$limitandoffset = $this->formatLimitAndOffset($limit, $offset);
	# make text query clause
	$lcletter = mb_strtolower($letter, 'UTF-8'); // convert to lower case, UTF-8 safe
	$langClause = $this->generateLangClause($lang);
	$textcondPref = $this->createTextQueryCondition($letter . '*', 'skos:prefLabel', $langClause);
	$textcondAlt = $this->createTextQueryCondition($letter . '*', 'skos:altLabel', $langClause);
	$orderbyclause = $this->formatOrderBy("LCASE(?match)", $lang) . " STR(?s) LCASE(STR(?qualifier))";
	$qualifierClause = $qualifier ? "OPTIONAL { ?s <" . $qualifier->getURI() . "> ?qualifier }" : "";
	$filterDeprecated="";
	if(!$showDeprecated){
	$filterDeprecated="FILTER NOT EXISTS { ?s owl:deprecated true }";
	}
	$query = <<<EOQ
	SELECT DISTINCT ?s ?label ?alabel ?qualifier
	WHERE {
	$gc {
	{
	$textcondPref
	FILTER(STRSTARTS(LCASE(STR(?match)), '$lcletter'))
	FILTER EXISTS { ?s skos:prefLabel ?match }
	BIND(?match as ?label)
	}
	UNION
	{
	$textcondAlt
	FILTER(STRSTARTS(LCASE(STR(?match)), '$lcletter'))
	FILTER EXISTS { ?s skos:altLabel ?match }
	BIND(?match as ?alabel)
	{
	?s skos:prefLabel ?label .
	FILTER (langMatches(LANG(?label), '$lang'))
	}
	}
	?s a ?type .
	$qualifierClause
	$filterDeprecated
	} $values
	}
	ORDER BY $orderbyclause $limitandoffset
	EOQ;
	return $query;
	}
	}

Before merging it should be discussed whether something else should be done to both of these SPARQL parts, namely, if there should be a more special treatment for "*" and whether or not the letter value should be changed to that very same value.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[codecov]

Codecov Report

Merging #1103 (254c2c6) into master (254c2c6) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1103   +/-   ##
=========================================
  Coverage     60.30%   60.30%           
  Complexity     1592     1592           
=========================================
  Files            32       32           
  Lines          4431     4431           
=========================================
  Hits           2672     2672           
  Misses         1759     1759           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 254c2c6...5293aca. Read the comment docs.

[osma]

The situation where this can happen is when a vocabulary has been configured to have a certain language, but there are actually no prefLabels with that language tag. The code in WebController tries to find the first alphabet letter for which prefLabels exist, but when there are no such prefLabels, the list is empty. This is basically a configuration error but these things happen, even in production as you've noticed by checking the logs.

I think we need a unit test to check for this situation. The suggested fix may be OK, I need to double-check. The sane result in this not-so-sane situation would be to show an empty list of alphabet letters as well as an empty alphabetical listing of concepts.

[sonarqubecloud]

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
23.1% Duplication

[osma]

I will merge this assuming the Travis CI tests pass. The SonarCloud complaint is irrelevant, it complains about identical blocks in unit tests (true, but harmless).

[osma]

Can't wait for Travis, but the tests pass locally, so merging this.