propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 399 forks source link

First beta release coming up #1733

Closed dereuromark closed 3 years ago

dereuromark commented 3 years ago

Hi Especially to the ones actively contributing: It is time we start to make a new release happen.

What do you think we should include in our first beta release? We should probably

Maybe you can help me here? And we can get a milestone planned, including release date?

mringler commented 3 years ago

Oh, exiting!

Some thoughts on the milestones:

Personally, I would like to see Propel 2 get out of dev-limbo, so descoping the open issues makes sense to me.

Hope that helps, best Moritz

dereuromark commented 3 years ago

I updated your post and added bullet points to toggle once done. Can you please check 1st item? The ticket seems to be a different one than you meant. Also, for the 2nd part, can you maybe also link the others in the same style, you can just edit your post.

This way, once we see all are done, we can close this ticket later a bit more easily.

SourceCode commented 3 years ago

Migrations in PHP seems like a really poor idea. Why and what would make a migration depart or need to depart from the native language of the storage engine you are using.

dereuromark commented 3 years ago

@mringler You mentioned some already merged PRs. It would be easier to focus only on open issues and PRs.

mringler commented 3 years ago

@SourceCode I think the idea was to be independent of database engine, for example when you use different dbms between your dev setup and production. And I guess it would look cool. And, in theory, it would make things easier for less SQL-savvy developers. But yeah, between the different SQL dialects, bugs in migration diff engine, bugs in the proposed migration processing engine, the expectation that users will learn a new set of commands, and the loss of transparency - I really don't see this going well, even if someone were to invest the two weeks of work I think it would take. It's probably one of those "wouldn't it be nice" ideas. It would, but it can't.

@dereuromark Guess I misunderstood your comment. I thought you wanted links to my other PRs. Pity. That's time I won't get back. Can you please clarify:

Also, for the 2nd part, can you maybe also link the others in the same style

What 2nd part, and what others?

dereuromark commented 3 years ago

Of my PRs...

Those "other" ones you were talking about but are not yet in master. Issues or PRs. Anything that should also go into the timeline for the beta.

Everything that is already in master doesn't need to be further considered in this ticket IMO. The goal is to have a [ ] checkbox for each missing item that we can partially tick off. We could also create milestones and assign them there, if that makes things easier - but not everyone has (write) access to those.

mringler commented 3 years ago

Ah, ok. So I have one last PR that I have just added. But it is just some random test fixes. Nothing important. After that, I am all PRd out. Seems (for now) everything I need is finally working.

So it is only my open PRs that I would be happy to see added. Those are:

marcj commented 3 years ago

@mringler

That's a fun one, but a ton of work. It says "max 1 day of work" in the description, and I seriously doubt that

even if someone were to invest the two weeks of work I think it would take.

Just a quick note here since I don't like the way you're trying to put it. Before you're trying to make my statements look invalid, you should better be precise with your words: It takes you 12 days to implement it. Quite obviously, when I stated it takes one day, that's referring to me. You can't doubt that at all since you lack critical insides of how I work. If you need 12 days instead, then that's fine, but you should say so, instead of unnecessarily making other people look like they are talking nonsense. You are making 3 gross mistakes here: 1. You assume our understanding of Propel and PHP is equal. 2. you are trying to put your speed on the statements of others. 3. you assume that 1 day is equal to the 8hour day in Germany, but I usually work 16h a day. What is obvious is that not only our working speeds are obviously massively different but also our workload per day. Therefore, you should refrain from making old statements look bad without knowing with absolute certainty all the details. Granted after not working with PHP anymore since many years, it would take me probably 2-3 days, but definitely not 2 weeks like you are trying to imply.

Also, it probably makes it harder to see and fix errors in generated migrations

And, in theory, it would make things easier for less SQL-savvy developers.

Not at all. If you prefer SQL, you can enable it via configuration option. Also the migration execution could print the SQL before execution. Nothing has to be intransparent except if you want to implement it that way.

But yeah, between the different SQL dialects, bugs in migration diff engine, bugs in the proposed migration processing engine, the expectation that users will learn a new set of commands, and the loss of transparency

It's probably one of those "wouldn't it be nice" ideas. It would, but it can't.

I get it, you're trying to put your opinion as the only truth because you don't like the task/feature or because you lack the knowledge/motivation to implement it, but not only are your statements obviously rather just an uninformed opinion but also partially plain wrong, plus such behaviour has no place in team collaboration and makes you unlikeable.

mringler commented 3 years ago

Wow, someone sounds angry. Lots of assumptions, accusations and even insults.

First off, it was not my intention to step on your toes. I don't really see how I did, since my tone was in no way personal or offensive. You actually quoted me, saying "this is a fun one" and "wouldn't it be nice", so I don't think I spoke as deprecating about the feature as you make it sound.

Our estimate on how long it would take to implement it obviously differs, but I am a bit flabbergasted how you seem to deal with another opinion. If you can do it in a day - great! I obviously notice that you haven't put in that one measly day in the last seven years, for a project, that seems to have meant a lot to you at some point, and a feature, that even today you feel very strongly about. But unless you do that, I feel it is moot to speculate how much quicker you can built it than anybody else. Personally, I am convinced that just writing proper tests for such a feature would take at least a day. But I don't mind being surprised and appreciative by how productive you are, once I see it. If it is necessary, let me stress that I said "personally" here, and in my initial comment it says "the two weeks of work I think it would take", making it obvious that I am talking about my opinion, not universal truths (which I thought was obvious, considering it is a comment). And let me also state the obvious, which is that I welcome disagreement and being part of a discussion, especially if it is uttered in an appropriate tone. Which, honestly, and with all due respect, is a bit lacking in your response. For example, I cannot really change who I am or how I communicate, and when you find me unlikeable for that, I have to (and can) deal with it. But it is not really productive to share this opinion, nor is this here really the place for that, is it?

Actually, I am not even sure what the intent of your comment was. Do you want to talk about technical details, or tell me how much you are offended, and think that I suck? Because you can't have it both ways. And you are making it really hard to do the former.

marcj commented 3 years ago

Not angry at all, just pointing out what I think, rather unemotional rationally. I'm also perfectly fine with a rough tone, so an "appropriate tone" (which is highly subjective without global definition) is not really needed. But you should step back from the personal attacks. Keep it rational.

I don't really see how I did, since my tone was in no way personal or offensive

but I am a bit flabbergasted how you seem to deal with another opinion.

I don't deal at all with your opinion or your tone at all, I deal with the assessment to my statement. With the attempt to make it look wrong. You quoted me, so expect a reaction from that person. I don't care about your tone. You didn't express an opinion to me, you expressed like its a fact with even quoting me. That's the issue, not all your emotional digging and provocations around that. It's not a battle about who does it faster. Nobody cares about that, so you shouldn't bring that topic on the table. But quoting people and saying "its wrong" or to "doubt that" is an attempt to invalide their statement, which you simply can't in this case since its an opinion about my pace. To make it perfectly clear for you since you don't seem (you actually said that you don't get it) to get it and try to attack me personally and taunt me instead:

It says "max 1 day of work" but I think it takes longer for me [or our team].

This is an example of a valid opinion, if you really feel the need to even quote me. It does not try to invalidate my statement.

It says "max 1 day of work" in the description, and I seriously doubt that.

This on the other hand is not a valid opinion. It looks like you have legit arguments about my statement which could lead to it being wrong, which you inherently can't have. If you say yourself making it obvious that I am talking about my opinion why even compare to my quote that is obviously an opinion too. Why not only quoting, why do you event "doubt" my statement. You can not doubt my statements as it's not only not meant for you but on top an opinion about an environment and entity you have zero knowledge over whatsoever. Thats the entire point.

I cannot really change [...] how I communicate

You can, and you should.

But it is not really productive to share this opinion, nor is this here really the place for that, is it?

Quite the contrary. It's the exact right place and it is productive. Even on Github is it appropriate to point out behaviour that people think is not right. Whenever I see wrong claims or misleading statements I react. When people quote me and try to claim the opposite or something vastly different I react. Granted it happened rather by accident since I don't follow or read all incoming emails, but when I see something I try to react.

dereuromark commented 3 years ago

To get back on topic:

dereuromark commented 3 years ago

Another topic that would need some discussion probably:

As every such change is usually a major later, it would be a good time to fix this up for the beta. This would help to provide a long term API then from there on.

mringler commented 3 years ago

Adding more param and return types

My two cents: I think adding type hints is great, but getting the beta out is more important. I have seen several questions on stackoverflow, where people are not sure which version to use, and sometimes are discouraged to use Propel because of the unclear situation, between a very old stable v1, an alpha v2 and an undefined/idle (I think) v3. See for example here or here.

So the big question is, how much effort is it to add them?

dereuromark commented 3 years ago

@mringler @nederdirk We are now going into the release phase for the beta Can you please check if any of the open PRs, specifically refactoring, is fitting into it? We have a merge deadline of mid of this week on this. We need final and full review on those.

I guess that also means no further types are added for param/return then from the looks.

mringler commented 3 years ago

Great news! Looking forward to the beta.

dereuromark commented 3 years ago

One remaining issue seems to be still a point of discussion for us

PR propelorm/Propel2#1711 introduced a lot of changes to the way SELECT columns are resolved by Propel's query builder (see ModelCriteria::select()).

As a side effect of this change the order of query builder's::select() and ::addAsColumnt() method calls now matters. All the alias columns used in a query MUST be specified prior to the ::select() method call. Otherwise, when provided to ::select() method aliases will be considered invalid column names and will lead to an exception being thrown.

This leads to BC-breaking changes across dozens (if not hundreds) of our Spryker modules.

I wonder if there is a way to relax this issue again.

dereuromark commented 3 years ago

@mringler Can you please quickly give your feedback on this? Should we untangle this order issue before we tag such a release?

mringler commented 3 years ago

@dereuromark Sorry for the delay, too much to do at the moment. Yes, we should fix this, I can do it later this week or over the weekend.

dereuromark commented 3 years ago

Is the following release notes draft OK as is?

### 2.0.0-beta1

With this release, we start Beta-version releasing towards API stabilization.

The main changes include:
- PHP 8.1 compatibility
- Fixes for PHP 7.4 preloading
- Fixed usage of default on-update and on-delete behavior
- Show names of uncommitted migrations
- BehaviorLocator now looks in dev packages, as well
- Aggregate multiple columns behavior & parameter list support 
- Fixes around aliases and cross joins and subqueries
- Added support for `keytype` in the magic import/export methods
- PSR naming fixes for variables and methods
- Reset partial flag when populating a relation
- Added `exists` operator
- Escape quotes in behavior
- Quote primary table name if identifier quoting is enabled 
- Formats insert `DATE` values as `Y-m-d` instead of `Y-m-d H:i:s.u`
- Allow default-value for concrete-inheritance to be instantiable
- Pluralize `Box` to `Boxes`
- Allow `NO ACTION` for foreign key references (in the dtd/xsd)
- Use object-equality instead of reference-equality to compare object properties
- Generates data dictionary documentation
- PHPStan related code cleanup

Many thanks to all the contributors!

#### BC breaking impact
Please note that methods have param and return types being added now where they were feasible and ensure better code quality.
Make sure any extensions are updated here regarding their method signature.
Some internal methods were also renamed to fit PSR coding standards.

If so, I will prepare final release for today.

mringler commented 3 years ago

@dereuromark : Looks great! For me, the most important updates are the PHP version fixes, enabling EXISTS, and fixing subqueries, since this affects core functionality and everyday usage. So I'd put those first. But that might be my own skewed perspective. Anyway is fine.

Great to see this moving forward! Sounds like we can pop some metaphorical champagne tonight, and wait for bug reports to come in. Pretty nice!

mringler commented 3 years ago

Oh, and maybe add a notice that an update will make a call to config:convert necessary. Something like

Due to the supporting of PHP 7.4 preloading, an update will need the configuration to be rebuilt once by calling config:convert, see https://github.com/propelorm/Propel2/wiki/Exception-Target:-Loading-the-database#for-imported-configuration

dereuromark commented 3 years ago

https://github.com/propelorm/Propel2/releases/tag/2.0.0-beta1 Thank you so much for your help!