[core] refactor: introduce php autoloader by dvikan · Pull Request #2508 · RSS-Bridge/rss-bridge

dvikan · 2022-03-18T03:09:11Z

One small step towards code base modernization.

This is a refactor that makes it easier to introduce an autoloader.

One small step towards adding an autoloader.

Moving file includes to a central location in lib/rssbridge.php makes it easier to introduce an autoloader later.

They are completely empty.

This change fixes the problem with loading files in the correct order.

Ideally these files should be located in some other dir.

There are a few requires in some bridges. But since they are require_once, it is not a problem.

dvikan · 2022-03-19T05:21:38Z

I think we are ready to introduce an autoloader now. Example:

spl_autoload_register(function ($class) {
	$folders = [
		__DIR__ . '/../actions/',
		__DIR__ . '/../bridges/',
		__DIR__ . '/../caches/',
		__DIR__ . '/../formats/',
		__DIR__ . '/../lib/',
	];
	foreach ($folders as $folder) {
		$file = $folder . $class . '.php';
		if (is_file($file)) {
			require $file;
		}
	}
});

Ping @f0086 @em92 @Bockiii @yamanq

Bockiii · 2022-03-19T22:48:36Z

Can you do a write up of what this will do? Like, the usecase behind it and then the general approach in prose?

Easier than to try to assess it from the code :)

dvikan · 2022-03-20T00:44:25Z

Autoloading is the process of automatically loading PHP classes without explicitly loading them with the require(), require_once(), include(), or include_once() functions.

Advantages:

Code isn't littered with include calls
No need to worry about dependency ordering of includes
Easier to introduce php namespaces (and later PSR4)
Easier to reuse parts of the application when testing in cli
Easier to merge with composer's autoloader when we introduce it later
Enforcement of one class per file (no mixing of responsibilities)

One last one is that the code base looks kinda ancient and old. Which it is. From a marketing and popularity standpoint as a FOSS project, I think we should start to modernize. Will attract more contributors.

em92 · 2022-03-20T19:27:27Z

bridges/MspabooruBridge.php

-require_once('GelbooruBridge.php');
-
-class MspabooruBridge extends GelbooruBridge {
-
-	const MAINTAINER = 'mitsukarenai';
-	const NAME = 'Mspabooru';
-	const URI = 'http://mspabooru.com/';
-	const DESCRIPTION = 'Returns images from given page';
-	const PIDBYPAGE = 50;
-


Why we delete this bridge and some others?

Ah this is a mistake on my part. I though this was a dead and empty skeleton bridge. I can see now that it inherits from another bridge.

em92 · 2022-03-20T19:32:15Z

bridges/DanbooruBridge_Fix_Simple_Html_Dom.php

+/**
+ * This class is a monkey patch to 'extend' simplehtmldom to recognize <source>
+ * tags (HTML5) as self closing tag. This patch should be removed once
+ * simplehtmldom was fixed. This seems to be a issue with more tags:


Since simplehtmldom is abandoned, I am considering to make this patch directly in vendor/simplehtmldom directory or fork simplehtmldom in RSS-Bridge/simplehtmldom. What do you think about it?

Since it's abandoned I think moving it to vendor/simplehtmldom is fine.

Maybe later we can seriously consider maintaining a fork under the rss-bridge organization. We probably have to live with this dependency a long time.

New bridges in the future should maybe start using:
https://symfony.com/doc/current/components/css_selector.html

or any other modern DOM crawlers.

If I remember correctly, this bug was addressed a long time ago. The monkey patch should no longer be necessary with the current version of simplehtmldom.

em92 · 2022-03-20T19:34:47Z

lib/ActionFactory.php

-		require_once $filePath;
-


Tbh, I don't understand why should we not use require_once in this case.

This commit move the include calls to lib/ and later an autoloader is introduced so it's not necessary to manually include files anymore.

em92 · 2022-03-20T19:35:48Z

lib/Debug.php

 		static $firstCall = true; // Initialized on first call

-		if($firstCall && file_exists(PATH_ROOT . 'DEBUG')) {
+		if($firstCall && file_exists(__DIR__ . '/../DEBUG')) {


Why can't we use PATH_ROOT here and on other places?

I guess this is mostly a matter of taste. I find the __DIR__ and __FILE__ more intuitive because they are file relative. Whereas the PATH_ROOT is unknown and must be looked up by new code reviewers.
In any case PATH_ROOT and __DIR__ . '/../' points to the same directory.

Also when we migrate to composer's autoloader later (I hope!) we can't use PATH_ROOT in composer.json to define where our classfiles are located.

I guess this is mostly a matter of taste. I find the __DIR__ and __FILE__ more intuitive because they are file relative. Whereas the PATH_ROOT is unknown and must be looked up by new code reviewers. In any case PATH_ROOT and __DIR__ . '/../' points to the same directory.

Matter of taste is not a good reason for change. __DIR__ . '/../' does not tell you anything about the path, except its location. PATH_ROOT (as well as PATH_LIB, PATH_CACHE, etc.) were introduced to improve readability of the codebase by adding semantic information. Please also note that it is much easier to find all references of PATH_ROOT than it is to locate the right place in all variations of __DIR__ . '/../...'.

That said, this change is not needed for the autoloader and should be removed from this particular PR.

Also when we migrate to composer's autoloader later (I hope!) we can't use PATH_ROOT in composer.json to define where our classfiles are located.

This is fine. Once composer's autoloader is implemented, most of the paths will vanish from the codebase anyway.

Reert mistake on my part. These are actually real bridges. I didn't notice that they extend other bridges. This reverts commit ef54d4a.

dvikan · 2022-03-22T20:14:38Z

Need more feedback regarding this change.

Bockiii · 2022-03-25T21:08:08Z

Did you check the monkey-patch-topic? logman said it may not be neccessary anymore.

lib/rssbridge.php

logmanoriginal · 2022-03-27T08:11:00Z

tests/BridgeImplementationTest.php

 		foreach (glob(PATH_LIB_BRIDGES . '*.php') as $path) {
 			$bridges[basename($path, '.php')] = array($path);
 		}
+
+		// Remove a few files that are not bridges
+		unset($bridges['PepperBridgeAbstract']);
+		unset($bridges['DanbooruBridge_Fix_Simple_Html_Dom']);
+


Bridges must have a Bridge suffix in their file name, so instead of removing unwanted files manually, perhaps this can be solved by applying the correct filter.

Suggested change

foreach (glob(PATH_LIB_BRIDGES . '*.php') as $path) {

$bridges[basename($path, '.php')] = array($path);

}

// Remove a few files that are not bridges

unset($bridges['PepperBridgeAbstract']);

unset($bridges['DanbooruBridge_Fix_Simple_Html_Dom']);

foreach (glob(PATH_LIB_BRIDGES . '*Bridge.php') as $path) {

$bridges[basename($path, '.php')] = array($path);

}

Bockiii · 2022-04-05T11:00:00Z

I think if you merge logmans suggestions, this can be given a go

dvikan · 2022-04-07T19:37:26Z

I'll make a new smaller PR.

dvikan added 10 commits March 18, 2022 04:05

[core] refactor: improve includes organization

e43ba1a

One small step towards adding an autoloader.

[core] refactor: extract class loader out of CacheFactory

3d5cf97

Moving file includes to a central location in lib/rssbridge.php makes it easier to introduce an autoloader later.

[core] refactor: centralize loading of output format classes

63c7754

[core] refactor: centralize loading of action classes

9d96a92

[core] refactor: remove skeleton bridges

ef54d4a

They are completely empty.

[core] refactor: extract class PepperBridgeAbstract

1c7a2a6

This change fixes the problem with loading files in the correct order.

[core] refactor: extract class

b6b0dda

[core] test: blacklist two files that aren't bridges

308844c

Ideally these files should be located in some other dir.

[core] refactor: centralize loading of bridge classes

36a2bd8

There are a few requires in some bridges. But since they are require_once, it is not a problem.

[core] test: remove unnecessary includes

d4683a6

dvikan changed the title ~~[core] refactor: improve includes organization~~ [core] refactor: prepare for introduction of an autoloader Mar 19, 2022

dvikan added 2 commits March 20, 2022 02:38

[core] refactor: finally introduce the autoloader

9cc1773

[core] fix: missed one file that contained functions

b9a9039

em92 reviewed Mar 20, 2022

View reviewed changes

Revert "[core] refactor: remove skeleton bridges"

aee720a

Reert mistake on my part. These are actually real bridges. I didn't notice that they extend other bridges. This reverts commit ef54d4a.

logmanoriginal reviewed Mar 27, 2022

View reviewed changes

lib/rssbridge.php Show resolved Hide resolved

logmanoriginal reviewed Mar 27, 2022

View reviewed changes

Bockiii mentioned this pull request Mar 28, 2022

[core] Update simplehtmldom to latest released #2556

Merged

dvikan changed the title ~~[core] refactor: prepare for introduction of an autoloader~~ [core] refactor: introduce php autoloader Apr 4, 2022

dvikan closed this Apr 7, 2022

dvikan mentioned this pull request Apr 13, 2022

refactor: add php autoloader #2655

Merged

Conversation

dvikan commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvikan commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bockiii commented Mar 19, 2022

Uh oh!

dvikan commented Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logmanoriginal Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvikan commented Mar 22, 2022

Uh oh!

Bockiii commented Mar 25, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bockiii commented Apr 5, 2022

Uh oh!

dvikan commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dvikan commented Mar 18, 2022 •

edited

Loading

dvikan commented Mar 19, 2022 •

edited

Loading

dvikan commented Mar 20, 2022 •

edited

Loading

logmanoriginal Mar 21, 2022 •

edited

Loading

dvikan commented Apr 7, 2022 •

edited

Loading