Implemented EZP-19411: Avoid file_exists and include calls for register...

[crevillo]

...ing datatypes and workflows

See https://github.com/ezsystems/ezpublish-legacy/commit/a6bfaa622342b728951c7f45bb066828440c3be3

[emodric]

This is a breaking change.

Please be aware that eZ Tags extension supports eZ Publish 4.4 and above.

[crevillo]

I don't understand... Why do you think it's a breaking change?

Edit: You're saying that we can't add this change because eztags supports ez 4.4?

[emodric]

@crevillo Well, there's no technological limitation that prevents eZ Tags from functioning on eZ Publish 4.4. Why change that?

[crevillo]

@emodric Well, i see your point. you're talking about the case somebody gets last master version from github repo while still using ez publish 4.4? I see your point,,, but maybe then we may need to revert the changes made to ezflow and others... i ping @andrerom about...

[emodric]

@emodric Well, i see your point. you're talking about the case somebody gets last master version from github repo while still using ez publish 4.4?

Exactly.

P.S. While we're at it, it looks like this fix has some problems of its own in eZ Publish.

http://share.ez.no/forums/install-configuration/ezp-2013.6-could-not-get-instance-of-datatype-problem

[crevillo]

gonna try a ezpublish 2013.5 install and then an 2013.6 to see if that problem appears. it shouldn't...

2013/8/20 Edi Modrić notifications@github.com

@emodric https://github.com/emodric Well, i see your point. you're talking about the case somebody gets last master version from github repo while still using ez publish 4.4?

Exactly.

P.S. While we're at it, it looks like this fix has some problems of its own in eZ Publish.

http://share.ez.no/forums/install-configuration/ezp-2013.6-could-not-get-instance-of-datatype-problem

— Reply to this email directly or view it on GitHubhttps://github.com/ezsystems/eztags/pull/63#issuecomment-22952540 .

Twitter : @crevillo Skype ID : carlostanta

Desarrollador en Tanta Comunicación. eZ Diff Squad Member

[crevillo]

Gonna update the forum topic to say that i have no problems with the upgrade at all. i did it two more times again, checked the content on the files and it work as expected.

Regarding eztags, i though master versions was meant to be used with latest builds. So, when 2013.08 or ezpublish 5.2 appear, this change will be included in eztags too.

Feel free to close this then or maybe wait until some other change in kernel could make eztags crash.

@andrerom, Do you think we could revert these changes in ezflow and others?

2013/8/20 Carlos Revillo crevillo@gmail.com

gonna try a ezpublish 2013.5 install and then an 2013.6 to see if that problem appears. it shouldn't...

2013/8/20 Edi Modrić notifications@github.com

@emodric https://github.com/emodric Well, i see your point. you're talking about the case somebody gets last master version from github repo while still using ez publish 4.4?

Exactly.

P.S. While we're at it, it looks like this fix has some problems of its own in eZ Publish.

http://share.ez.no/forums/install-configuration/ezp-2013.6-could-not-get-instance-of-datatype-problem

— Reply to this email directly or view it on GitHubhttps://github.com/ezsystems/eztags/pull/63#issuecomment-22952540 .

Twitter : @crevillo Skype ID : carlostanta

Desarrollador en Tanta Comunicación. eZ Diff Squad Member

Twitter : @crevillo Skype ID : carlostanta

Desarrollador en Tanta Comunicación. eZ Diff Squad Member

[emodric]

I don't see a good reason why newer versions of extensions would be reserved for newer builds of eZ Publish.

[crevillo]

well, i can't discuss that. I just followed what is has been approved as a new way to define datatypes and workflow in order to avoid those include_once calls. But it's your extension, it's your choice. No problem with that. Cheers.

[emodric]

I'll let @andrerom decide. It's @ezsystems repo after all.

[gggeek]

I'm not an official maintainer of anything, so I can not decide, either, but we have stable branches of extensions which track stable releases of eZ. That should take care of supported customers.

As for CP users:

In the long run we should imho have a better build system, where we "tag" extensions which are to be used with each CP build as well, and maybe support patchsets / backports for each branch.

This is a longstanding need we never properly addressed in fact - not enough hands in the CP Board for that - but in the meantime we switched (started to) to composer, which removes the needs for builds and embraces branches/tags.

And to be brutally honest, I'm pretty sure there have been other changes done in the "trunk" of other ez extensions which have more-or-less broken compatibility with older ez versions - in fact the extensions which take more care about being backwards compatible with a single trunk are the community ones (I'm qualified to attest to that ;-) )

Which means +1 for me even if it is a BC. Just document it properly.

[andrerom]

I'm somewhere between -1 and +1 on this, I can clearly see the benefit of maintaining one version of the extension for several eZ Publish versions, and as long as this extension is not bundled with EE, there will not be a new stable branch for every EE version which would imply a -1. However:

@crevillo Would it be possible to change the PR to both allow 2013.07 and higher to use autoload as well as allowing it to work on older versions? If the kernel feature was slightly changed to not break the format this could have worked as only change in this PR:

-AvailableDataTypes[]=eztags
+AvailableDataTypes[eZTagsType]=eztags

If the feature instead accepted this format we would have avoided the break some people experience right now. What do you think? Other options?

[emodric]

-AvailableDataTypes[]=eztags
+AvailableDataTypes[eZTagsType]=eztags

@andrerom I would go for this solution, but in that case, eZDataType::register call needs to remain in all the datatypes in case the old way is still used.

[crevillo]

@andrerom i'll take a look, but i first, i don't think it can be done if not changing something in the kernel itself... is that what you're saying?

Besides that, let me add i don't really understand the goal of this. imho, improvements on the kernel can be added to the kernel as long at it doesn't break extension behaviour. this is the case.

I have no examples right now, but probably we could find some *inevitable" BC changes in our ez publish history that have implied modifications in the extensions to make them work.

Finally, are we sure that this master version of eztags works with ez4.4, as provided in http://share.ez.no/downloads/downloads/ez-publish-community-project-4.4-fuji, even without this change?

i ask this because of this change. https://github.com/ezsystems/eztags/commit/26da0617fbd5e636de510f1af368017350e05ccc

There, jquery-migrate is loaded... but where is that file in ez4.4? if i'm not wrong, jquery-migrate it's only 5 months old in ezjscore repo...

Haven't tested, so probably i'm missing something...

[andrerom]

@emodric This is what I implied by ".. this could have worked as only change in this PR", meaning skipping the other parts of the patch.

@crevillo Yes, that is exactly what I imply, changing the kernel a bit more to avoid breaking the ini format.

But fair point regarding jquery migrate, I'll leave it to @emodric to comment on that but would assume it might change the position on both this and the admin -> admin2 discussions.

[emodric]

But fair point regarding jquery migrate, I'll leave it to @emodric to comment on that but would assume it might change the position on both this and the admin -> admin2 discussions.

Fair point indeed, but that change does not break backwards compatibility since the extension will still work. In case jquery migrate is missing, the only thing that will happen is that warning will be appended to the log.

[crevillo]

Fair point indeed, but that change does not break backwards compatibility since the extension will still work. In case jquery migrate is missing, the only thing that will happen is that warning will be appended to the log.

Well, in that case, i haven't much more to say regarding the convenience of this pr.

To finish, for me this is like saying the symfony devs to not do this change https://github.com/symfony/FrameworkBundle/commit/bbede10727830df6bf1e116216183accd4c5f579, because ez 5.0 uses the old config name. What we'll have to say to ee customers is "please don't use last symfony version with your 5.0 install because it will not work " am i right?

to me, we add changes to the kernel trying to avoid bc changes in the extensions as in this case. What we don't do is keeping extension free of changes because maybe they won't in previous ez versions.

To me, ez publish kernel is "boss" of ez ezystems as symfony is "boss" of ez5 versions :).

Other than that, the only way to make extensions work in old and new versions is just not adding this change to them...

[crevillo]

thinking about it, what about backporting those kernel change to old ez versions? at least to all ez versions having autoload capabilities...

Crude but...

Edit: Also, please note this was implemented long time ago, but was forgotten. If i remember well, when the improvement was implemented at first it was supposed to be added to 4.5, and it worked well...

[crevillo]

Any other opinions on this? Let me add another thought.

eZ Systems encouraging everybody to contribute. In my opinion, contribution will be harder if we force contributions to work with previous versions of the kernel. Contributors are people that are giving eZ System part of their (in some cases free) time to try to innovate, fix things and so. If we force them to test previous versions whenever they add a change to an extension, i think they will discourage.

Btw, There's nothing in the contribution guidelines asking you for that (as far i can see). Anyway, if this is going to the policy, i think it should be stated which versions of the kernel should be taken in account. That could include community versions too...

Here we are talking about ez 4.4, but someone could perfectly ask .. ."hey, and why not 4.2 too?"

Just my two cents.

[crevillo]

closing this. feel free to reopen.