Skip to content

Conversation

AndrewKostka
Copy link
Contributor

No description provided.

Magnus Manske and others added 28 commits June 11, 2021 14:49
within run_single_command, the site name is checked against the currently available sites. This was done, using the functions strtolower() and trim() to clean up the file name. The only problem is that at no other point in the app are the site names adjusted this way, causing issues, when the site name uses upper case letters or spaces in the beginning and end. 
In wmde/wikibase-release-pipeline#293 I suggested using the same variable to full the site name, as the label, go show to the user which wikibase instance the user is going to write to with the current quickstatement page. For this branding however, users might be inclined to use a name that contains capital letters causing Quickstatements to fail when writing to the API.
removed strtolower() and trim() on site name check
These break the API when using later verisons of PHP8 as deprecation warnings are printed.
wmde/wikibase-release-pipeline#398 (comment)

As this is hardcoded and can't be overwritten by the php.ini file it's probably worth having
a default that is less broken
api.php, ignore E_DEPRECATED errors
documentation of ```format``` use in public_html/vue_components/user-page.html
This was broken by 8d516de. The new $statement field 'new_statement'
is checked via isset() in getStatementID(), but unconditionally assumed
to exist in compressCommands(). However, only importDataFromV1() was
taught to create this field, while importDataFromCSV() wasn’t. This
meant that parsing CSV statements would produce a PHP warning, which is
included in the web response and thus breaks the JSON output.

Fixes #38.

An alternative would be to guard the access in compressCommands(); the
easiest option would be the `??` operator, but that was only added in
PHP 7.0 and I don’t really want to risk breaking sites that use
QuickStatements on older PHP (even though PHP 7.0 has been out for a
while already).
If parseValueV1() can’t parse the value, the $cmd it returns will still
have a 'datavalue', but there won’t be a 'value' inside it (but rather a
'text', along with 'type'=>'unknown'). This is apparently not checked at
any intervening point (neither in the V1 nor CSV formats), and
eventually the commandAdd*() methods would try to send the incomplete
data to the API, after producing several PHP warnings due to the missing
value (several of them inside getSnakType()), which would also break the
JSON response as usual.

Work around this by checking if we have a value just before sending the
API request. This means that the bad values will still be shown in the
batch view in a weird format (raw JSON), but I don’t know how that
should be fixed instead. This at least fixes the server-side warnings
and makes the batch not stuck indefinitely.
Fix statement creation in CSV mode
Don’t try to send incomplete values to the API
Basically the same as commit 4622f78, just for another API action.
No Wikibase API needs entity IDs with namespaces ("Item:Q24").
fix: Deprecation warning & float to int conversion
@deer-wmde
Copy link
Contributor

looks good to me - lets merge it so we get an image built we can test on staging

@AndrewKostka
Copy link
Contributor Author

looks good to me - lets merge it so we get an image built we can test on staging

@deer-wmde it's possible to test pull requests on staging before merging for this repository. This one is published as sha-46017a1 from https://github.com/wbstack/quickstatements/actions/runs/13238854400/job/36949528189#step:8:1009.

@deer-wmde
Copy link
Contributor

I added a fix for the upstream contribution I did earlier, I already included it here as well

magnusmanske/quickstatements#60

@AndrewKostka AndrewKostka merged commit ae0ee47 into main Feb 12, 2025
3 checks passed
@AndrewKostka AndrewKostka deleted the upstream-sync-20250210 branch February 12, 2025 13:04
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.

8 participants