Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Phalcon\Mvc\ModelInterface to include getModelsMetaData(). #13070

Closed
wants to merge 2 commits into from

Conversation

virgofx
Copy link
Contributor

@virgofx virgofx commented Sep 14, 2017

Fixed Phalcon\Mvc\ModelInterface to include getModelsMetaData(). This is important for static analysis as the ModelInterface is used as the interface parameter for various plugins and user components.

@virgofx virgofx force-pushed the 3.2.x branch 7 times, most recently from d2f54a0 to 902a3b9 Compare September 14, 2017 19:43
@virgofx
Copy link
Contributor Author

virgofx commented Sep 14, 2017

ping @sergeyklay . Need me to submit 4.0.x also?

@sergeyklay
Copy link
Contributor

I'll cherry pick this to the 4.0.x a bit later if the need arises. Thank you for contributing. Could you please return back the changes in the phalcon/mvc/model.zep related to import classes.

@virgofx
Copy link
Contributor Author

virgofx commented Sep 14, 2017

The 2 imports that were removed were:

use Phalcon\Validation\Message\Group;
use Phalcon\Validation\Message\Group as ValidationMessageGroup;

They are both not used + duplicated. The other imports were reordered alphabetically.

What are you suggesting? To re-add the above imports, despite them being not used? Everything works/builds . Shouldn't need to re-add unused imports ...

@sergeyklay
Copy link
Contributor

Actually I prefer to keep it as is (unordered) and just add new ones to decrease diff.

@virgofx
Copy link
Contributor Author

virgofx commented Sep 16, 2017

It's not about the diff.... It's about correctness. 2 of the imports are not used and shouldn't be imported.

@Jurigag
Copy link
Contributor

Jurigag commented Sep 19, 2017

Btw, shouldn't this change aim 4.0.x? #12676 i will add it there Oh wait we will do anyway changes to ORM so i guess this method will be deleted. To be honest i don't even know if we need this change.

@sergeyklay
Copy link
Contributor

sergeyklay commented Sep 19, 2017

@virgofx I see 14 additions and 17 deletions in this file and would like to see only 2 deletions.

@sergeyklay
Copy link
Contributor

@virgofx Removed not used imports in the #13089. Could you please rebase

@virgofx
Copy link
Contributor Author

virgofx commented Sep 29, 2017

Rebased .. but left alphabetized as that's how coding standard is. The reason for the extra deletion is because there was another duplicate Phalcon\Mvc\Model\Message which was missed as result of poor diffing tool (not my fault). Imports should always be optimized/alphabetically as this is consistent with the entire code baseline. Please merge.

@sergeyklay sergeyklay added this to the 4.0.0 milestone Oct 2, 2017
@sergeyklay
Copy link
Contributor

sergeyklay commented Oct 2, 2017

We can't change any interface in minor release. Thus I added this to the 4.0.0 milestone.

@sergeyklay
Copy link
Contributor

Also just to you know there is no benefit from ordering imports in code which will be compiled and optimized before release

@virgofx
Copy link
Contributor Author

virgofx commented Oct 2, 2017

I agree compiled code doesn't care; however, the coding standard and those contributing all follow best practices. No reason not to keep things inline best practice. If this is for the 4.0 milestone ... will you fix in a separate PR then with the rest of the interfaces?

@virgofx
Copy link
Contributor Author

virgofx commented Oct 2, 2017

Best practice would also have moving a lot of the constants from Classes into Interfaces as well.

If this won't be merged do you want me to submit to 4.0 or will you handle with interface cleanup everywhere in 4.0? If so, want me to close?
ping @sergeyklay

@virgofx virgofx closed this Oct 7, 2017
@sergeyklay
Copy link
Contributor

@virgofx Could you please send this PR to the 4.0.x branch?

@virgofx
Copy link
Contributor Author

virgofx commented Jun 5, 2018

I could but is 4.0 selectively cherry picking commits? Seems like it would miss core functionality from 3.x? Just making sure other 3.x stuff is also in 4.0?

@sergeyklay
Copy link
Contributor

The 4.0.x branch is actual now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants