Better code analysis with @method tags.

[rudolfbyker]

Since you are wrapping File_MARC_Record and friends in Record.php and Field.php, and use __call to delegate the method calls to them, you should have @method tags to aid IDE code hinting. Here are examples for Record.php and Field.php:

/**
 * The MARC record wrapper.
 *
 * @property string id
 * @property string type
 *
 * @method string getLeader() Get the leader.
 * @method ... more of these
 */
class Record implements \JsonSerializable {
//...
}

/**
 * Class Field
 *
 * @method string toRaw() Get the raw MARC data.
 * @method ... more of these
 */
class Field implements \JsonSerializable {
// ...
}

Another option would be to get rid of the __call magic methods and just let the user use the wrapped object directly, through getField or getRecord. That would need proper type documentation, too:

/**
 * Get the wrapped field.
 * 
 * @return \File_MARC_Field
 */
public function getField() {
    return $this->field;
}

This method already exists in Field.php but not in Record.php. Here is an example of what we need in Record.php:

/**
 * Get the wrapped record.
 * 
 * @return \File_MARC_Record
 */
public function getRecord() {
    return $this->record;
}

We could also do both. I'll even make a PR if you tell me which solution you prefer.

[rudolfbyker]

An even cheaper option would be to make the protected $field and $record fields public, and document them properly:

/**
 * The wrapped field.
 * 
 * @var \File_MARC_Field 
 */
public $field;

[danmichaelo]

Thanks for bringing this up, @rudolfbyker. This is lazyness from my side, I agree that adding @property and @method hints would be a good thing! Especially since the interface for the File_MARC package isn't very likely to change.

And yes, we can also make $record public or add a getRecord() method, for those who prefer not using the magic. I don't really have an opinion on which of the two is better.

If you would like to go ahead and prepare a PR, great!

[rudolfbyker]

OK, I'll prepare a PR. Are you using a specific code style or code analysis tool? Regardless, I'll try to make it consistent with the rest of your code.

[rudolfbyker]

Also, what's the oldest PHP version you are targeting?

[danmichaelo]

Thanks!

I'm using phpcs to align with PSR2, see ruleset.xml.

I should also probably document this, but if you use overcommit, it will run cs checks and linting before each commit, see .overcommit.yml

As for PHP version, this project officially still supports PHP 5.6, but I'm open for changing that to 7.1 or 7.2 if you see the need for it!

[rudolfbyker]

Version

Perfect. I'll stick to PHP5.6 for now, since file_marc is also targeting PHP 5. (For my own projects, I have dropped all support for PHP 5.* .)

Code style

PHPCS is great. My IDE actually detected the rules before I realised they are specified in ruleset.xml.

Problem

We have a slight problem with adding the @method tags for the Field class. It wraps File_MARC_Field, which has a few methods which we can annotate, but File_MARC_Field also has subclasses. I see we have a specific wrapper class ControlField, but I don't find a DataField wrapper class, and yet we have methods like getSubfieldValues in Field.php that assume that it's wrapping File_MARC_Data_Field and not File_MARC_Control_Field.

This is a code smell that I won't try to fix in this PR, but we have to decide whether we will annotate the methods of File_MARC_Data_Field in Field.php even though they will not be available when it's actually wrapping a control field.

I think we should, and then just add a FIXME note.

Alternatively…

If this were my project, I would drop the __call and __get methods completely, and force everyone to use getField()-> and getRecord()->. That makes everything a whole lot cleaner, but it would be a breaking change, which would require a new release that is not backward compatible. Not sure if that's important to you.

It's still a possibility for a later release, since I'm adding getRecord anyway, and getField is already there.

[danmichaelo]

(Btw. since you asked about code style etc. I took the opportunity to add a CONTRIBUTING file since this project didn't have one.)

[danmichaelo]

The idea of this project was to create a more fluent / less verbose interface to File_MARC using some magic, so I think it might defeat the purpose a bit if we enforce a more strict typing scheme.

Would it help to add a new DataField wrapper? Many of the classes under https://github.com/scriptotek/php-marc/tree/master/src/Fields could extend that. And the makeFieldObject() method could probably be refactored to return either ControlField or DataField depending on tag.

[rudolfbyker]

Yes, I think that would be structurally more sound, but I also think it's a separate issue from this one. Also not sure how one would go about doing that in a backward-compatible manner. Maybe that is something for v3.

[danmichaelo]

Indeed, it would be something for 3.0, but it could probably be done in a way so that code that doesn't depend on the result being a specific type would not break :)

Agree it's a different issue, I created #17 for it.