aimeos / aimeos-core

Aimeos PHP e-commerce framework for ultra fast online shops, scalable marketplaces, complex B2B applications and #gigacommerce
https://aimeos.org
Other
3.25k stars 112 forks source link

[FEATURE] rename classes / interfaces to include information about the class #77

Closed AndreasA closed 4 years ago

AndreasA commented 8 years ago

Currently all classes are called like Standard, Base, Factory, Iface. That makes working with an IDE way more complicated, e.g. with PhpStorm there is a good autocomplete that checks for class names and even a search by classname option. It would also automatically add corresponding use statements.

So if the the class for the Order would be called OrderStandard (or depending if it is used in aimeos-typo3 OrderTypo3) one could easily find the correct class by searching for "Order". That, however, is not so simple with only the namespaces.

In regards to backwards compatibility it would be relatively simple to achieve by adding corresponding class_aliases.

aimeos commented 8 years ago

This will make some work because we have a lot of classes and renaming can't be done automatically. Are you able to provide some help?

AndreasA commented 8 years ago

In theory yes but not currently (have some other tasks to finish first). Maybe around november or later. If I have time earlier I can let you know. However, it is not necessary to rename all classes at once, it can be an incremental change as long as we support backwards compatbility to the old class names.

We would also need to consider how to do the renaming (e..g as mentioned just adding e.g. "Basket" in front of the Iface and the Standard class or something lese?) and the class_alias backwards compatibilty configuration because I doubt that one would work automatically either. But probably the simples solution is a config array in that regard and define all aliases upon start,

Or something similiar to the typo3 class alias loader, where the classloader takes care of it, though one cannot ensure a TYPO3 installation so it would have to be an own class loader?

aimeos commented 8 years ago

This is for the first 2017 version and the time frame for those changes would be from October to end of December.

AndreasA commented 8 years ago

Which means all classes would have to be renamed by then?

aimeos commented 8 years ago

Yes, that's the time frame for those kind of (breaking) changes

AndreasA commented 8 years ago

Hmm.. Not sure how much time I will have until then. Otherwise we could always add those changes in 2018 version?

Regarding breaking if the class_alias are defined, it actually shouldn't be breaking :)

But it is a major change so I still agree there in regards to only with major release :)

aimeos commented 8 years ago

A first step could be to change the interface from ...\Iface to ...\BasketIface and leave the rest as is. Then, you can search for the interfaces easily and that's what's probably looked up most often.

AndreasA commented 8 years ago

But even then we would need the class_alias support in place first because if someone uses the old interface in their own code that should still work I guess?

gilbertsoft commented 7 years ago

And here my 5 cents: for me it was very difficult in beginning despite a quick comprehension and long-term programming experience (not php, but other oop). After some months I almost know the structure of the project, but would be nice having meaningful names for classes and interfaces. As you said it's very important to have aliases, if not you loose the complete backward compatibility and everyone has to fix his whole code! On the other hand the replacement of some code by customer is much more easier like that, e.g. see https://aimeos.org/docs/Configuration/Core/client/html/basket/mini/name. But could be that only depends ai-html-client?

AndreasA commented 7 years ago

I think the issue developed when aimeos moved from "_" separator in classnames to namespaces. So the classes earlier were probably something like ..._Product_Standard renamed to ..\Product\Standard.

I am wondering if one couldn't rename them automatically, after all \Product\Standard should be renamed to \Product\ProductStandard the same for Iface.

If not it will be quite a lot of work but it definitely should be done.

gilbertsoft commented 7 years ago

Try http://textpad.com/products/wildedit/index.html ;-)

AndreasA commented 7 years ago

The problem is one also needs to modify all references to it which is quite tricky because it is possible that classes have been referended by using "use" that way only part of the full classnames are there. Not to mention the namespace tag also has to be taken into account. So a regular expression is prboably not enough for this.

PhpStorm can do renames nicely with finding references but the problem is that would need to be done per file.

Another problem might be to include a backwards compatibility classloader (as this is a major change or make everything breaking for 2018 version) like TYPO3 uses that adds class alias maps to the composer autoloader (might be usable for that actually).

aimeos commented 7 years ago

Yes, it's a major change so it will be only possible for the 2018 Aimeos series. From our point of view, changing the names is possible end of this year without caring much about backward compatibility.

Personally, I would only rename the interfaces as they are what's used in IDEs. The classes are not that important and renaming them as well would be very much work, not only for the classes itself but also for the documentation.

AndreasA commented 7 years ago

The classes should alao be renamed. E.g. when one wants to create an instance of a class tge IDE does NOT show interfaces. So tge best solution would be to rename both ibterface and classes.

faerietree commented 7 years ago

AndreasA wrote:

I am wondering if one couldn't rename them automatically, after all \Product\Standard should be renamed to \Product\ProductStandard the same for Iface. If not it will be quite a lot of work but it definitely should be done.

Please my friends, please do not rename from Product\Standard to Product\ProductStandard.

Reasons:

Please save your and our (Aimeos admins') time for more important matters (of which there are many in the software world and within Aimeos, e.g. automatic bundle pricing). (Of course I will not stop you, you are free and that's good! Logic should prevail though and not personal development setup issues.)

You as developers know pretty well how fragile code and the world in general is.

aimeos commented 7 years ago

What do you mean with the term "automatic bundle pricing"?

faerietree commented 7 years ago

Created a feature request. #92


@ Rename issue

Discussed this issue's rename request internally on the @worlddevelopment club board. Conclusion:

Even without such renames we fight with many merge conflicts and no longer working functionality customizations after every Aimeos update. e.g. template customizations, search functionality and account emails. That's not Aimeos' fault, that's our fault - or rather the companies' special desires, you know that every company wants its shop to work in a very special way ;-) . It's just that I fear there'll be much more breakage after such questionable mass renames. Please what do you think?


Aimeos vs. Gambio

Else in any way - in my opinion - Aimeos has overtaken Gambio shop system. It's my clear number one and I'll deploy it on up to four more TYPO3 systems.

Also we at @worlddevelopment will donate parts of our income to Aimeos once we get our eco shop going. We do this because we are open, want to be fair and because Aimeos is very honorable because in comparison to pseudo open source Gambio it is really very open and friendly. (Requested Gambio sources of their version 3 over 1 year ago and still did not get it. That's actually a verdict against their License.)

Thanks to the Aimeos Team. :-)

AndreasA commented 7 years ago

In regards to namespaces: Actually if I want to use "new" in most cases one doens't want to find X\Y when searching for X. Really, seldom that one wants to be able to find by namespace there but it could be an optional feature. Also aimeos uses various class make sense, e.g. Basket plugins or in setup the ProductAdd ... Now, if one tries Product those where the last part of the class name is something with product will most likely always be at the top of the list.

In regards to customizations no longer working: This is why my original idea was to define the old class names as alias using class_alias to the new class names. That way if there are any old class references they will be perfectly fine (interface and class), the TYPO3 class-alias loader actually supports class aliases out of the box.

faerietree commented 7 years ago

In regards to namespaces: Actually if I want to use "new" in most cases one doens't want to find X\Y when searching for X. Really, seldom that one wants to be able to find by namespace there but it could be an optional feature.

That still sounds more like a PHPStorm issue, because this can be solved by adding a restriction option in the PHPStorm search|class autocomplete. Also the damage of more entries to choose from should be minimal. (e.g. they could be greyed out or use a different color to clearly signal that this autocomplete match was found by including the path.)

It seems we have to trade the hassle + redundancy[1] that comes with the rename versus the usability gain in some IDEs (or request the namespace|path feature at the IDE).

[1] (related to what @gilbertsoft said about easier configuration, e.g. less to type and less to change on creation of new functionality|options)

Also aimeos uses various class make sense, e.g. Basket plugins or in setup the ProductAdd ... Now, if one tries Product those where the last part of the class name is something with product will most likely always be at the top of the list.

Couldn't we simply include the immediate parent folder in the selection list? Then the Product... items also should be showing first in the list.

faerietree commented 7 years ago

In regards to customizations no longer working: This is why my original idea was to define the old class names as alias using class_alias to the new class names.

OK. That's good. The overhead should not be that impactful then. Though will the alias be put in the new file or reside in the file with the old name? (The primer sounds more sensible than the latter due to less filesystem clutter and Git patching still working if git mv was used to move the files.)

aimeos commented 7 years ago

@faerietree You are right, the problem is an IDE issue but the situation is as it is the case. I'm unsure if we want to rename all classes and therefore, the suggestion was to rename the interfaces only for IDE support.

@AndreasA If you instantiate a class using "new" in Aimeos, you did something terribly wrong ;-)

aimeos commented 7 years ago

There will always be breaking changes in versions released in the beginning of the year. This is necessary to get better quickly and to clean up the code of things that didn't work. The good news: They are only rarely that big as in this year after we've radically simplified the templating. But it was necessary because old template system limited the chance of Aimeos being used widely.

As every 20xx.10 version is an LTS release, you can stay for almost two years on a 20xx series (from 20xx.01 to 20xx.10 + one year LTS support). You can even use it up to five years with support by the Aimeos company: https://aimeos.com/support/#c781

If you want to upgrade afterwards, at least the database upgrades are rather smooth due to the setup tasks updating your database automatically to the new version.

faerietree commented 7 years ago

If you want to upgrade afterwards, at least the database upgrades are rather smooth due to the setup tasks updating your database automatically to the new version.

I agree. And even if the database update script breaks, then it is easily patchable as seen in the pull request #91.


As said, you are free to decide collaborately about the rename and I am happy we discussed this openly and in a friendly way. :-) I can live with the compromise though objectively it's still not making sense to tailor code to certain IDEs. If too many issues arise though, the Aimeos core can still be patched or forked.

Aimeos is the only mostly open source, lightweight, modular e commerce solution that supports TYPO3 and where direct update using Git is possible as to our knowledge.

I've seen universities and companies alike dwell on deprecated software for so long that it becomes a major security threat and later huge update struggle costing hundreds of thousands of euros -- but well, each to their own. We for our case prefer rolling release.

Noone can say there are no solutions of freedom around for OSes, web servers, eCommerce|ERP and even hardware. Noone can complain this world does force them to do this or that or to spend money.

AndreasA commented 7 years ago

@aimeos The "new" was just an example but the issue still applies :) Although there are some instances in aimeos where this applies, e.g. adding Helpers to aimeos View or throwing exceptions or creating the payment form or Attribute\Cirteria etc.

In that case let's make it PHPdoc. Or trying to access the "::class" variable or a check with instanceof etc. etc. When using PHPdoc the IDE tries to add the class by name and automatically adds a use Statement.

Here, we have a problem that really can only be easily solved by renaming the classes because the IDE cannot differentiate between all those Iface or Standard classes Now the IDE cannot add multiple use statements easily because all classes or interfaces end basically with "Standard" or "Iface"

AndreasA commented 6 years ago

Apparently the main reason behind this issue might be fixed by PHPStorm in a soon upcoming release, see: https://youtrack.jetbrains.com/issue/WI-24527 It isn't released yet but it is already in EAP. It is supposed to be fixed in 2018.2 version. If that is the case, I guess we can just close the ticket. Of course, it will not fix the autoimport issue as those would still result in multiple "Standard" but I don't think there will be that many "cross" imports and one can just manuall define the "use" statement or use FQDN.