opencart / opencart-3

OpenCart 3.x
Other
25 stars 31 forks source link

Create missing Ref definition #228

Closed AJenbo closed 10 months ago

AJenbo commented 10 months ago

With out this the check in system/engine/proxy.php will always fail, but since it appears you want to keep it the only other solution is to create the symbol.

TheCartpenter commented 10 months ago

Not quite ideal since the system/framework would also have to load the new proposed Engine file rather than creating a protected method from the proxy constructor.

AJenbo commented 10 months ago

The alternative is to delete the check, it appears to just be a vestige that always results in the else branch being taken and so should be safe to remove.

TheCartpenter commented 10 months ago

OC 4 no longer has it instantiated either. Perhaps the engine should be upgraded as well without pulling the addPath method for the extension. Doing so, however, would require some modifications to the startup folders. At least, the vendors packages have recently been upgraded on this tree as well compared to the previous run.

By just deleting this check, I am not sure if that would really solve the issue since the convention names are, or were, also depending on this instance from Proxy in the past for the controllers. If it'd have to be removed, how would those conventional names be as compatible without the Ref and getRef()?

AJenbo commented 10 months ago

I don't see where Ref is ever defined, if that is the case then the check for it can safely be defined. The only issue I could imagine is if some extension made there own class called Ref but then you could only have one such extension I would think.

In short I think this is the right solution: https://github.com/opencart/opencart-3/pull/227

Else you will have to show me a more concrete example of where that code would be active.

TheCartpenter commented 10 months ago

I don't see where Ref is ever defined, if that is the case then the check for it can safely be defined. The only issue I could imagine is if some extension made there own class called Ref but then you could only have one such extension I would think.

In short I think this is the right solution: #227

Else you will have to show me a more concrete example of where that code would be active.

It's not loaded anywhere else on the platform. There are no other concrete example that can be demonstrated on this one.

The Proxy class is the only one loading that instance but this class also propagates throughout the entire platform as well that's why I also asked on the above if we'd rather have to upgrade the engine due to deprecated instance. Since, as you said, it can affect extensions and the entire goal is for the users to keep running the platform with the current extensions they have until they're ready to upgrade to OC 4.x releases.

AJenbo commented 10 months ago

but this class also propagates throughout the entire platform

It's only ever used internally in that class and not exposed in it's signature so it shouldn't really cause any issues to remove it.

Since, as you said, it can affect extensions

I think it's very unlikely that any extension would have done so as they would have to define a class that is missing from core for things to work. I really think it's safe to remove this.

TheCartpenter commented 10 months ago

Now, to deal with the event. This repository does not, yet, deal with namespaces from the Engine. Only from the library files for OC v3.x releases until the next outcome.

AJenbo commented 10 months ago

Now, to deal with the event

Done: https://github.com/opencart/opencart-3/pull/230