Tweak: depracation notices and type hint properties.

[dpanta94]

Fixes deprecation notices.
makes declarations be enforced by type hinting.
ignores generated files from repo.
Amends changes in tests.
adds docs and imports.

[JasonTheAdams]

Thanks, @dpanta94! Am I correct that this is based off of the #22 branch?

[dpanta94]

Yes @JasonTheAdams you are correct. I am building on top of you code and also i am bringing in here the schema updates from here

[JasonTheAdams]

This is really neat, @dpanta94! Thanks for this!

I'm really tempted to suggest this should be it's own SchemaModels repository. The main reason is that this is strongly coupled to Schema, of course. If we had a middle-step solution that introduced concepts such as primary keys and the like without coupling it to Schema, that would be attractive because it would flexibly work in different systems. Our own products, such as GiveWP, don't use Schema, so this would add Schema as an unused dependency.

I think simply having another library that depends on both packages could be a good idea. It keeps this lean and less opinionated. An alternative would be to flip this and have it be in Schema, making this package a dependency.

[dpanta94]

@JasonTheAdams thanks for your comments. This PR simply enables integration with another stellarwp library, schema. It doesn't add any dependency, you simply can continue using the rest of the library (anything other than SchemaModel) like you used to.

If it happens that your project use both models and schema like we do in TEC, you can take advantage of the existing integration between 2 stellarwp libraries which i find very logical and honestly something to be expected.

[JasonTheAdams]

To be clear, @dpanta94, I think this is a great bridge feature. My question is whether it should be in this package, in the Schema package, or in its own package entirely. It's not quite true to say "it doesn't add any dependency" — it adds the Schema package to composer, which means that everywhere using this package will now also load that one. Schema also requires initialization, which adds a bit of risk (though I don't think this requires it to be initialized in every case).

Personally, I think it makes the most sense for this to live in Schema, because I think there's a higher chance that if someone is using Schema they're also using Models, whereas there's significant use case where someone is using Models without using Schema.

[dpanta94]

@JasonTheAdams i could see it going that way, but i think you might be confused because of the diff. Stellarwp/schema is required only as a dev dependency and that is only to test it. When you will require stellarwp/models in your project you will not get the schema package along with it. So i still believe it does not add any dependencies. It's up to the developer using it whether they want to integrate it with schema or not.

[JasonTheAdams]

That doesn't make sense, we're using classes from Schema (e.g. use StellarWP\Schema\Tables\Contracts\Table_Interface;) in the SchemaModel class. As such, it should be a prod dependency, not a dev dependency. Even if it's considered optional, it's still required for this package to work in its entirety.

[dpanta94]

@JasonTheAdams personally i think it does make sense. If we were creating a WC integration, we would not require the whole WC codebase. We would register based on if it's loaded or not.

Similar here, we are adding an integration for schema. If you don't have schema required in your project you can't use the SchemaModel interface, but if you do have it you can. If you try to use it when you don't have it, you will get a fatal very fast, loud and clear in your development phase.

[borkweb]

I think there are good points being made here by both @dpanta94 and @JasonTheAdams. Initially, I was in complete agreement with @dpanta94 - the overhead of a separate repository to provide the interface, the abstract class, and the tests seems like overkill. The developer experience is straightforward - they won't see errors if they don't use the interface or class.

However.

The developer experience can be pretty bad (and maybe dangerously so) if the developer builds out the code with dev dependencies and then assumes the Schema library is available when it actually isn't. It would definitely result in fatals in production if the developer never ran the code without --no-dev and just packaged up for production.

So - after chewing over it a bunch, I think a SchemaModels library is the right plan and that can be done fairly rapidly. Then if someone wants to include both libraries, they'd actually only need to include the SchemaModels one in their composer.json and it'd bring in its hard dependencies - models and schema.

[borkweb]

Here's the new repo: https://github.com/stellarwp/schema-models

[dpanta94]

I think this approach is better considering your very well though argument with the scenario that someone may be developing while requiring all dependencies.

Tomorrow i will move the SchemaModel work to the new repo. We would still need to merge #22 of this repo though and release v2.0.0. I can create a PR that is also bringing that across the finish line.

Thank you both @JasonTheAdams and @borkweb 🖖 !