https://bugzilla.wikimedia.org/show_bug.cgi?id=63224
--- Comment #7 from Chris Steipp <[email protected]> --- Moritz, please keep discussion here so we have the conversation all in one place. (In reply to Chris Steipp from comment #5) > In general, please follow the style guide (unless Wikibase has it's own) This wasn't copied to https://github.com/Wikidata-lib/PropertySuggester/issues/70, and I'm guessing wasn't addressed. > The build directory shouldn't be web accessible-- is this meant to be > removed in production? Otherwise, add an htaccess rule for it. Fixed. > The code uses PropertyId::newFromNumber, which looks like isn't supposed to > be used. Dacry said, "PropertyId::newFromNumber is not supposed to be used, because usually one shouldn't care about (numeric) ids but we naturally do - so i think we agreed that it should be ok in our special case ..." Are wikidata folks ok with that? I want to make sure the Wikibase interface doesn't change in a way that will break this, if no one is supposed to be using that function. > ./PropertySuggesterHooks.php > * It seems like the JS doesn't need to be added on every page view Fixed (I'll assume that's the right way in wikibase to do that) > ./maintenance/UpdateTable.php > * Not a security issue, but using do { } while would make the code more > readable. Fixed. > ./src/PropertySuggester/Suggesters/SimpleSuggester.php > Line 75 - please escape $minProbability xchrdw said "minProbability is checked to be float in line 56", which isn't the point. Please follow [[Security_for_developers#Demonstrable_security]] and escape as close to the output as possible. Additionally, is_float may not be sufficient (I honestly don't have time to try and exploit your implementation). > Please add profiling fixed > ./src/PropertySuggester/ResultBuilder.php > Nitpick, but the "createJSON" function returns an array. Naming consistency > makes reviewing easier. fixed > ./src/PropertySuggester/GetSuggestions.php > The performance of using continue + limit as your db limit seems like it may > cause issues. It's efficient, but running profiling will tell if it's a > serious issue. Dacry said, "I do not really see issues caused by using continue + limit as your db limit. We did already do some profiling and performance seems to be fine" Can you point add or point to your profiling data? > ./src/PropertySuggester/UpdateTable/Importer/MySQLImporter.php > * For production, imports need to be broken up into chunks. Talk with Sean, > but Asher used to recommend importing in blocks of about 10k to avoid > overloading the slaves. xchrdw said, "the basic importer uses chunks and is the default importer now". That's fine. Are the deployment issues documented anywhere, so that whoever at the wmf/wmde who is deploying this will know not to override the default? > ./src/PropertySuggester/SuggesterParamsParser.php > Line 48 - test is_null first Fixed > ./PropertySuggester.php > Please don't require_once /vendor/autoload.php Dacry said, "requiring autoload.php ist needed for testing purposes and it is only included if an autoloader exists. Is the way of doing it problematic / does a better way exist? ;)" If it's only needed for testing, can you also check that PHP_SAPI == 'cli'? We should minimize places where an exploit that allows creating a new file can be turned into remote code execution. -- You are receiving this mail because: You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
