class rewrite

[Detzler]

There are some firegento classes which rewrite core classes but don't extend from them. So pontentially this could lead to an undefined method error.

E.g:
https://github.com/firegento/firegento-pdf/blob/development/src/app/code/community/FireGento/Pdf/Model/Shipment.php#L28

It should be:
class FireGento_Pdf_Model_Shipment extends Mage_Sales_Model_Order_Shipment

Shouldn't it?

[Schrank]

IF it should be extended from Mage_Sales_Model_Order_Pdf_Shipment, but til now, there is no reason to do this. Do you have one?

[Detzler]

I bet a crate of beer on the fact that there are definitely extensions or even core classes which call the methods of the "Mage::getModel('sales/order_shipment')" model. Since Firegento/Pdf redeclares this class through a rewrite, it's not possible anymore to call the parent methods because FireGento_Pdf_Model_Shipment doesn't extend from the sales/order_shipment class. This case is not that improbable.

[Schrank]

we are NOT rewriting sales/order_shipment' but `sales/order_pdf_shipment``

[Detzler]

My fault sorry, that's what I meant. There are totally 3 rewrites. 2 of them don't extend:

FireGento_Pdf_Model_Invoice
FireGento_Pdf_Model_Shipment

[Schrank]

Just to make this clear, you are complaining about something you have no clue whether this is a problem or not, right?

Come back if you have a problem please :-)

[Detzler]

First at all, I don't complain.. I just opened an issue. If you don't see it as a bug, then take it as an enhancement.
Actually, I have a concrete clue, which you don't take seriously. I think we shouldn't wait until this class rewrite behavior leads to an error page in a production system. (You won't even never know if it's has been happened)
If my description is not clear at all, I can be more specific. Just give me precise feedback, instead of making usage of the YAGNI principle, which can be applied basically on nearly every software issue.

[Schrank]

Just to make sure: didn't meant complaining as whiny, english is only a foreign language for me, didn't want to offend you.

I take it seriously. My problem is, I don't now which consequences there are and if there is no reason (and as long as no one is reporting a problem, there is no reasin imho) to add more complexity, we sohuldn't.

[Detzler]

Thank you for your clarification :)

as long as no one is reporting a problem

Well, here I am :D

Let me show you a real world problem:

Two extension are installed which rewrite "sales/order_pdf_invoice". It ends with a rewrite conflict (Amasty wins):

Current inheritance:
class FireGento_Pdf_Model_Invoice
class Amasty_Orderattr_Model_Order_Pdf_Invoice extends Mage_Sales_Model_Order_Pdf_Invoice

To satisfy all the other classes, which expect that "sales/order_pdf_invoice" is an instance of FireGento_Pdf_Model_Invoice, I need to fix the inheritance chain.
So I extend Amasty_Orderattr_Model_Order_Pdf_Invoice from FireGento_Pdf_Model_Invoice.

Consequently, Amasty_Orderattr_Model_Order_Pdf_Invoice needs to inherit the methods from Mage_Sales_Model_Order_Pdf_Invoice, but it can't because FireGento_Pdf_Model_Invoice extends nothing.

Can't be more detailed...

[Schrank]

It seems we have a problem, which needs to be fixed.