Page MenuHomePhabricator

wikimedia/avro returns many voids and other issues
Closed, InvalidPublic

Assigned To
None
Authored By
Reedy
Nov 20 2015, 1:30 PM
Referenced Files
F2991308: PhpVoidFunctionResultUsedInspection.xml
Nov 20 2015, 5:31 PM
F2991305: PhpUnusedParameterInspection.xml
Nov 20 2015, 5:31 PM
F2991297: PhpMissingParentCallCommonInspection.xml
Nov 20 2015, 5:31 PM
F2991307: PhpUndefinedMethodInspection.xml
Nov 20 2015, 5:31 PM
F2991292: PhpDocMissingThrowsInspection.xml
Nov 20 2015, 5:31 PM
F2991304: PhpUnusedPrivateFieldInspection.xml
Nov 20 2015, 5:31 PM
F2991303: PhpUnusedPrivateMethodInspection.xml
Nov 20 2015, 5:31 PM
F2991299: PhpMultipleClassesDeclarationsInOneFile.xml
Nov 20 2015, 5:31 PM

Description

No idea where this belongs...

/**
 * @param AvroSchema $writers_schema
 * @param AvroIOBinaryDecoder $decoder
 */
private function skip_data($writers_schema, $decoder)
{
  switch ($writers_schema->type())
  {
    case AvroSchema::NULL_TYPE:
      return $decoder->skip_null();
    case AvroSchema::BOOLEAN_TYPE:
      return $decoder->skip_boolean();
    case AvroSchema::INT_TYPE:
      return $decoder->skip_int();
    case AvroSchema::LONG_TYPE:
      return $decoder->skip_long();
    case AvroSchema::FLOAT_TYPE:
      return $decoder->skip_float();
    case AvroSchema::DOUBLE_TYPE:
      return $decoder->skip_double();
    case AvroSchema::STRING_TYPE:
      return $decoder->skip_string();
    case AvroSchema::BYTES_TYPE:
      return $decoder->skip_bytes();
    case AvroSchema::ARRAY_SCHEMA:
      return $decoder->skip_array($writers_schema, $decoder);
    case AvroSchema::MAP_SCHEMA:
      return $decoder->skip_map($writers_schema, $decoder);
    case AvroSchema::UNION_SCHEMA:
      return $decoder->skip_union($writers_schema, $decoder);
    case AvroSchema::ENUM_SCHEMA:
      return $decoder->skip_enum($writers_schema, $decoder);
    case AvroSchema::FIXED_SCHEMA:
      return $decoder->skip_fixed($writers_schema, $decoder);
    case AvroSchema::RECORD_SCHEMA:
    case AvroSchema::ERROR_SCHEMA:
    case AvroSchema::REQUEST_SCHEMA:
      return $decoder->skip_record($writers_schema, $decoder);
    default:
      throw new AvroException(sprintf('Uknown schema type: %s',
                                      $writers_schema->type()));
  }
}

skip_(boolean|int|float|double|string|bytes) all return the return of a void

skip_long is protected (and doesn't return anything)

skip_(array|map|union|enum|fixed|records) are all undefined

Event Timeline

Reedy raised the priority of this task from to Needs Triage.
Reedy updated the task description. (Show Details)
Reedy added subscribers: Reedy, bd808.

AvroNotImplementedException doesn't seem to exist

/**
 * @var AvroDatumWriter
 */
private $datum_writer;

AvroDatumWriter too...

I could keep going on. Just running the PhpStorm inspections on the repo throws a whole host of issues

Tagging with MediaWiki-Debug-Logger for the lack of a better project. This lib is being used for tying Monolog to Kafaka for Discovery-ARCHIVED today and for MediaWiki-Action-API "soon".

The files we have right now were ripped out of the upstream https://github.com/apache/avro project using git subtree to make a Composer package. Any fixes we make should probably be contributed back to the parent Apache project too. I did the expedient thing in forking but it might be worthwhile to talk to upstream about having them own a proper package in the long term.

I was going to suggest that we maybe just didn't pick up all the files we needed. But if you, for example, search for the missing exception on github...

https://github.com/apache/avro/search?utf8=%E2%9C%93&q=AvroNotImplementedException

It's just the throw new usages :(

If you look at the commit history here (files 4-5 years old) you can either decide that this a a very stable and focused utility library or that it was a thing somebody threw together to prove a point and then never looked at again. I'm sort of leaning towards the latter interpretation.

Maybe it was a good idea to fork after all. Now we just need to follow through with a thorough cleanup and add some missing features (T114733#1815581).

These are all the issues that PhpStorm sees:

Looks to be worth investigating. The missing methods appear to all be on the decoding side of things which I didn't test, chances are we won't be doing any kind of avro decoding from php so not sure it's worthwhile to spend much time fixing that. If we are going to maintain a fork, perhaps we just rip out the decoding portion?

PHPStorm applied phpdoc cleanup is https://gerrit.wikimedia.org/r/254435

There's also various private members used in AvroDataIOWriter

else
{
  $dfr = new AvroDataIOReader($this->io, new AvroIODatumReader());
  $this->sync_marker = $dfr->sync_marker;
  $this->metadata[AvroDataIO::METADATA_CODEC_ATTR] = $dfr->metadata[AvroDataIO::METADATA_CODEC_ATTR];

  $schema_from_file = $dfr->metadata[AvroDataIO::METADATA_SCHEMA_ATTR];
  $this->metadata[AvroDataIO::METADATA_SCHEMA_ATTR] = $schema_from_file;
  $this->datum_writer->writers_schema = AvroSchema::parse($schema_from_file);
  $this->seek(0, SEEK_END);
}

Anything on the right hand side of $dfr....

Change 254494 had a related patch set uploaded (by BryanDavis):
Implement AvroNotImplementedException

https://gerrit.wikimedia.org/r/254494

Change 254494 merged by jenkins-bot:
Implement AvroNotImplementedException

https://gerrit.wikimedia.org/r/254494

Change 254504 had a related patch set uploaded (by BryanDavis):
Make some AvroDataIOReader memebers public

https://gerrit.wikimedia.org/r/254504

Change 254504 merged by jenkins-bot:
Make some AvroDataIOReader members public

https://gerrit.wikimedia.org/r/254504