PHP 7.3 Strict Standards

[MikeyGMT]

PHP 5.6 in Admin > file_inspector then you find entries in error_log file in the admin folder.

PHP Strict Standards: Declaration of e_tree_model::load() should be compatible with e_front_model::load($id = NULL, $force = false) in e107_handlers/model_class.php on line 3449

class e_front_model extends e_model

2509

/**
     * Generic load data from DB
     * @param boolean $force
     * @return e_front_model
     */
	public function load($id=null, **$force = false**)
	{

class e_tree_model extends e_front_model

3194

/**
	 * Default load method
	 *
	 * @return e_tree_model
	 */
	public function load(**$force = false**)

Adding the ID Parameter seems to eliminate the error_log message, not sure what impact it could have.

Secondly bit more involved... not sure what to do with this one.

PHP Strict Standards:  Declaration of e_front_tree_model::update() should be compatible with e_tree_model::update($from_post = true, $force = false, $session_messages = false) in e107_handlers/model_class.php on line 3583

class e_front_tree_model extends e_tree_model

public function update($field, $value, $ids, $syncvalue = null, $sanitize = true, **$session_messages = false**)

class e_front_model extends e_model

public function update($from_post = true, $force = false, **$session_messages = false**)

[CaMer0n]

yep. These as nasty bugs in the heart of the e107 class structure. "Fixing" them has the potential to break a LOT of stuff. I think we'll need @myovchev to give some feedback before modifying this. I managed to eliminate about 4 or them some time ago, but these are trickier.

[MikeyGMT]

I'm adding $id=null, to 3194 and leaving it there for a while to see if it helps.

public function load($id=null, $force = false)

[MikeyGMT]

Good explanation here... http://stackoverflow.com/questions/3115388/declaration-of-methods-should-be-compatible-with-parent-methods-in-php "This message means that there are certain possible method calls which may fail at run-time. Suppose you have .... "

[MikeyGMT]

Line 504: class e_model extends e_object

Line 1842: class e_front_model extends e_model
Line 2786: public function update($from_post = true, $force = false, $session_messages = false)

Line 3048: class e_tree_model extends e_front_model
Line 3444: public function update($from_post = true, $force = false, $session_messages = false)

Line 3453: class e_front_tree_model extends e_tree_model
Line 3520: public function update($field, $value, $ids, $syncvalue = null, $sanitize = true, $session_messages = false)

The functions in extended classes need to have the same parameters as the parent class.
Do we add the extra params in the child function to the parent class?

Please can you advise @myovchev?
Thanks
Mikey

[rica-carv]

@MikeyGMT

The functions in extended classes need to have the same parameters as the parent class.

Not quite right, you can override a function on a child class with different parameters.
You can do it, but you shouldn't.....

[MikeyGMT]

OK, thanks @rica-carv fair enough. Just getting lots of error_log files whilst debugging LANs work. If it's nothing to worry about that's fine. :) Just need to clean the files out from time to time.

[Moc]

@CaMer0n What's the status on this one?

[DBSdevelopment]

After starting to utilize cron.php for an installation the following errors started to be generated in the installations root error_log - Appears to be part of the issue above. (PHP Version 7.1.17)

PHP Warning: Declaration of e_tree_model::load($force = false) should be compatible with e_front_model::load($id = NULL, $force = false) in /e107_handlers/model_class.php on line 3520

PHP Warning: Declaration of e_front_tree_model::update($field, $value, $ids, $syncvalue = NULL, $sanitize = true, $session_messages = false) should be compatible with e_tree_model::update($from_post = true, $force = false, $session_messages = false) in /e107_handlers/model_class.php on line 3654

[myovchev]

A refactoring attempt available in https://github.com/e107inc/e107/tree/bugfix/2146-model-refactoring

[CaMer0n]

I'm interested to get your thoughts on Miro's branch. @Deltik @SimSync .

[Deltik]

@CaMer0n: The strict warnings are a sign that the things in ./e107_handlers/model_class.php aren't abstracted properly.

Although @myovchev's refactoring may squelch the warnings, there are still abstraction problems.

For example, e_tree_model and e_front_model still implement completely different update() methods despite having a common ancestor. There is no substitutability between those two classes. Perhaps there should be an interface that abstracts away some of the details of these update() methods so that clients using objects of the e_model-based classes don't have to think about the implementation.

In my opinion, @myovchev's change is not nearly radical enough. The only way I can see this code get better is if we create an object-relational mapping that has minimal dependency on the rest of e107 and a clean interface… and then plug in the old e_model-like classes into this new clean interface. To retain backwards compatibility, we'd have to write tests for all of the paths in the e_model-based classes and finally refactor these classes to use the new interface. And all the references elsewhere in e107 eventually need to be updated to use the new interface so that we can phase out the mess that is e_model.

Of course, this is an enormous amount of effort, but how else would we make e107 a cleaner codebase?

[CaMer0n]

@Deltik I believe I have committed a partial fix. Please let me know what you think.

[Deltik]

@CaMer0n:

I guess what you mean by partial is this lingering issue:

Declaration of e_tree_model::load($force = false) should be compatible with e_front_model::load($id = NULL, $force = false), Line 3774 of /home/deltik/public_html/e107_handlers/model_class.php

The inheritance hierarchy goes e_tree_model < e_front_model < e_model. From what I can tell, e_tree_model breaks the method signature and seems to implement a very different load() method.

To fix this one, consider how this load() method was intended to be used and stick with one meaning.

It appears to me that e_tree_model is conceptually different from e_front_model. If I'm reading it right, e_tree_model seems to represent a whole table (or at least, multiple rows in a table) whereas e_front_model represents one row in a table, hence the $id argument.

If I could redo the e107 ORM, I would make e_tree_model an "e_collection", which represents multiple rows, and e_front_model would be an "e_model", which already exists and represents just one row.

Unfortunately, the way that the "collection" concept has mixed with the "model" concept means that any attempt to work around this mismatched declaration would just make the code worse. e_tree_model really needs to be separated into a "collection" concept.

[CaMer0n]

Thanks @Deltik !! :-) I'm thinking renaming load to loadBatch or loadMultiple() would be easier at this stage than a rewrite.

[Deltik]

How about e_tree_model::loadCollection()?

[CaMer0n]

It's nice, but it seems a little inconsistent to me with the terminology already used. We're talking about loading multiple rows, right? Currently when we edit those rows we call it a batch edit. What do you think?

[Deltik]

Okay, fair. loadBatch() sounds the best to me, then.

[CaMer0n]

👍 I'm not sure what kind of cascading effect this could have on other methods. Hopefully nothing that would be used by third-party plugins/themes.

[rica-carv]

@CaMer0n Unfortunatelly, you can't control what third party plugins do, so isn't better to just leave a remark on this build history regarding this renaming?

[CaMer0n]

@rica-carv That's all fine and well, until it's a third-party plugin on your site that fails and breaks your whole site after upgrading e107. I'm just looking for the least 'damaging' change possible.

[CaMer0n]

@Deltik I made the change by renaming to loadBatch(). Hopefully nothing broke. If you spot anything unusual, please let me know. Thank you.

[CaMer0n]

@Moc @Jimmi08 @tgtje This change has the potential to break a few things. (particularly news-related items). Please keep a look out. Thank you.

[Jimmi08]

@CaMer0n
feature box on frontend shows nothing now
no themes are available in theme manager (I tried to switch to bootstrap3 because of FB)
PHP 7.0.

[Moc]

Confirmed theme manager issue ("No records found")
News section appears to be fine for me so far. Pages/menus as well.

[CaMer0n]

@Moc @Jimmi08 Thank you. I think any news issues would be in the frontend. (menus, grid, etc).
Fix for Theme Manager coming shortly.

[Moc]

Theme menu indeed fixed. So far, no issues with news on admin area or frontend.

[CaMer0n]

@Jimmi08 Featurebox should be working now.

[Jimmi08]

@CaMer0n Confirmed, featurebox works now.

[Jimmi08]

@Moc Could you confirm this? It worked before this change. Problem with correct layout for page .

How to test this:

• install bootstrap3 theme and forum plugin
• by default jumbotron full layout is set only for forum plugin - it works (body has correct class)
• as soon as you add there next setting (f.e. forum, page) it changes to default layout

PHP 7.0.
If this is not related with this, I can create new issue.

[Deltik]

@Jimmi08: I noticed that your screenshot has a comma (,) after forum. Try removing that comma.

[Jimmi08]

@Deltik Thanks. Good eye. Now I need to find how I got it there... yes, this was problem, but I didn't see it before.

[CaMer0n]

@Jimmi08 Check theme.xml - it's a common mistake.

[Moc]

I think this one can be closed. Feel free to let me know if there's still an issue related to this, and I'll re-open.