Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add namespaced classes and PSR-4 support #711

Merged
merged 3 commits into from
Feb 28, 2022

Conversation

Art4
Copy link
Contributor

@Art4 Art4 commented Dec 21, 2021

I would like to have support for PSR-4. I saw in this issue that this change would be appreciated, so I looked into it.

If I understand PSR-4 correctly, there MUST be a namespace:

  1. A fully qualified class name has the following form: \<NamespaceName>(\<SubNamespaceNames>)*\<ClassName>
    1. The fully qualified class name MUST have a top-level namespace name, also known as a "vendor namespace".

However, SimplePie does not provide a namespace at the moment. This PR creates a class alias for each class, which is inside the namespace SimplePie. (For SimplePie_Parse_Date you can use SimplePie\Parse\Date). There are some exceptions to this:

  • The class SimplePie has the alias SimplePie\SimplePie.
  • Class SimplePie_gzdecode has alias SimplePie\Gzdecode (with capital G).
  • Class SimplePie_Core is deprecated and has not received an alias
  • Class SimplePie_Decode_HTML_Entities is deprecated and has not received an alias

Then I created a new src folder and created pseudo-classes in it that extend the old classes. The reason for this is that in composer.json you have to specify a folder for PSR-4 where the classes are located in the correct file structure. If one request the SimplePie\SimplePie class, the file src/SimplePie.php will be loaded. Thanks to class_exists('SimplePie'); the file library/SimplePie.php gets loaded (via psr-0) and there the class alias will be set. (The procedure is the same that was used for Twig, see twigphp/Twig#2484).

Additionally, I created simple tests in the tests\Unit folder that verify that the classes/interfaces are available.

What can the future look like?

The new src folder will allow us to gradually move all non-deprecated code into the namespace in the future. The plan for this might look something like this.

  1. Create pseudo-classes with namespace in the src folder and create aliases in the library folder. (This PR)
    class_alias('SimplePie_Cache', 'SimplePie\Cache', false);
  2. if possible use internally only the alias classes in the namespace.
  3. move the code from the library classes to the namespace classes
    1. create aliases in the src folder: class_alias('SimplePie\Cache', 'SimplePie_Cache');
    2. leave only pseudo-classes or deprecated code in library folder
    3. set deprecation warnings in these old classes
  4. next major version: delete deprecated code, i.e. the library folder, remove aliases

What do you think of this?

Automation script

For the modification and creation of the classes and tests I created a script based on the script from Twig:

<?php

$dryRun = true;

if (is_array($argv) && in_array('--non-dry-run', $argv)) {
    $dryRun = false;
}

$oldDir = __DIR__.'/library/';
$newDir = __DIR__.'/src/';
$testDir = __DIR__.'/tests/Unit/';
$header = file_get_contents(__DIR__.'/build/header.txt');
$i = 0;

$deprecatedClasses = [
    'SimplePie_Core',
    'SimplePie_Decode_HTML_Entities',
];

foreach (new RecursiveIteratorIterator(new RecursiveDirectoryIterator($oldDir,  RecursiveDirectoryIterator::SKIP_DOTS), RecursiveIteratorIterator::LEAVES_ONLY) as $oldFile) {

    $newFQCN = substr($oldFile, strlen($oldDir), -4);
    $oldClass = strtr($newFQCN, '\\', '_');

    $classType = 'class';
    if (in_array($oldClass, ['SimplePie_Cache_Base'])) {
        $classType = 'interface';
    }

    $newFilebase = strtr($oldClass, '_', '/');
    if (strpos($newFilebase, 'SimplePie/') === 0) {
        $newFilebase = substr($newFilebase, 10);
    }
    $newFilebase = str_replace('gzdecode', 'Gzdecode', $newFilebase);

    $newFile = $newDir.$newFilebase.'.php';
    $newFQCN = strtr($oldClass, 'SimplePie_', 'SimplePie\\');
    if ($newFQCN === 'SimplePie') {
        // Rename: SimplePie => SimplePie\SimplePie
        $newFQCN = 'SimplePie\\SimplePie';
    } else if ($newFQCN === 'SimplePie\\gzdecode') {
        // Rename: SimplePie_gzdecode => SimplePie\Gzdecode
        $newFQCN = 'SimplePie\\Gzdecode';
    }

    $newClass = substr($newFQCN, 1 + strrpos($newFQCN, '\\'));
    $newNamespace = strtr($newFQCN, ['\\'.$newClass => '']);

    $testFile = $testDir.$newFilebase.'Test.php';
    $testFQCN = strtr($newFQCN, ['SimplePie\\' => 'SimplePie\\Tests\\Unit\\']).'Test';
    $testClass = $newClass.'Test';
    $testNamespace = strtr($testFQCN, ['\\'.$testClass => '']);
    $i++;

    echo("\n-------- File ".$i.": --------\n\n");
    echo("Old file : $oldFile\n");
    echo("Classname: $oldClass\n");
    echo("\n");

    if (in_array($oldClass, $deprecatedClasses)) {
        echo("$oldClass is deprecated and will be ignored.\n");

        continue;
    }

    echo("New file : $newFile\n");
    echo("FQCN     : $newFQCN\n");
    echo("Namespace: $newNamespace\n");
    echo("Classname: $newClass\n");
    echo("\n");
    echo("Test File: $testFile\n");
    echo("Test FQCN: $testFQCN\n");
    echo("Namespace: $testNamespace\n");
    echo("Classname: $testClass\n");

    if ($dryRun) {
        continue;
    }

    // Create namespaced file
    if (!is_dir(dirname($newFile))) {
        mkdir(dirname($newFile), 0644, true);
    }

    file_put_contents($newFile, <<<EOTXT
$header
namespace $newNamespace;

{$classType}_exists('$oldClass');

if (\\false) {
	$classType $newClass extends \\$oldClass
	{
	}
}

EOTXT
    );

    // Add class alias
    file_put_contents($oldFile, <<<EOTXT

class_alias('$oldClass', '$newFQCN', false);

EOTXT
        , FILE_APPEND
    );

    // Create test file
    if (!is_dir(dirname($testFile))) {
        mkdir(dirname($testFile), 0644, true);
    }

    file_put_contents($testFile, <<<EOTXT
$header
namespace $testNamespace;

use PHPUnit\Framework\TestCase;

class $testClass extends TestCase
{
	public function testNamespacedClassExists()
	{
		\$this->assertTrue({$classType}_exists('$newFQCN'));
	}

	public function testClassExists()
	{
		\$this->assertTrue({$classType}_exists('$oldClass'));
	}
}

EOTXT
    );
}

if ($dryRun) {
    echo "\n########## dry-run mode ##########\n";
    echo "\n";
    echo "No files were modified or created.\n";
    echo "\n";
    echo "Run `{$argv[0]} --non-dry-run` to perform the changes.\n";
}

@Art4
Copy link
Contributor Author

Art4 commented Jan 15, 2022

Any thoughts or questions? Is there anything I could improve?

@mblaney
Copy link
Member

mblaney commented Jan 23, 2022

hi @Art4 I don't mind this patch, only hesitation would be that SimplePie focuses on remaining compatible with other projects so it would be good to hear how that will happen. I don't think issuing warnings for not using new namespaced classes is a good idea if that's what you meant.

@Art4
Copy link
Contributor Author

Art4 commented Jan 23, 2022

I would recommend to set silenced deprecation errors. They could look like this:

@trigger_error('The `SimpelPie` class is deprecated and will be removed in 2.0. Use `SimplePie\SimplePie` instead.', \E_USER_DEPRECATED);

Without the @-silencing operator users would see the errors as you noted. But with the @-silencing operator users would need to opt-in for deprecation notices.

set_error_handler(function ($errno, $errstr, $errline, $errfile) {
    // handle silenced errors
    if (0 === error_reporting()) {
        // throw Exception or show warning
    }
});

Silencing allows users to opt-in when they are ready to cope with the warnings and test the new classes. Then, when they no longer receive warnings, they know their code is ready for SimplePie 2.0.

However, setting the warnings is only optional and can also be omitted. But by doing so, we miss a good opportunity to inform the users about the planned changes and they would have a more difficult job to prepare early for the code changes with v2.0.

This forward compatibility concept has also been used by Symfony for many years and has proven very effective.

@mblaney
Copy link
Member

mblaney commented Feb 5, 2022

yeah I'm not sure. Maybe if enough people are keen on this then we could create a version 2.0 starting with this patch? I don't have the time to maintain it at the moment as it seems like there's quite a bit of refactoring to make it worthwhile.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 5, 2022

This change is specifically designed not to introduce a BC break so bump of minor version should be fine per semver. We could probably introduce something like https://github.com/Roave/BackwardCompatibilityCheck to CI to confirm compability.

@Art4
Copy link
Contributor Author

Art4 commented Feb 5, 2022

Maybe if enough people are keen on this then we could create a version 2.0 starting with this patch? I don't have the time to maintain it at the moment as it seems like there's quite a bit of refactoring to make it worthwhile.

I would strongly advise against introducing namespaces in a v2 branch. As you said, I also suspect that this feature will require a lot of refactoring. If this happens in a dedicated branch, then it becomes necessary to regularly merge the changes from the v1 branch into the v2 branch. This is even more time consuming and I personally don't feel like doing this extra work.

However, I am willing to continue working on refactoring from time to time, as I described in the plan in my first post. This way we are not dependent on someone having time, but can do the refactoring piece by piece and release it regulary. The namespaces in this branch could also be declared experimental for my sake. This way, users will always have the opportunity to use and test the namespaced classes and give us feedback if something is not working yet.

You also have to keep in mind that the introduction of the namespaces will bring other changes that are hard to estimate now. e.g. the constants, building with php build/compile.php etc.

But when we are sure that the namespaced classes are ready, we can mark the non-namespace classes as deprecated and remove them with v2. This makes the most sense, especially considering that this project is very much about stability and compatibility.

library/SimplePie/Exception.php Show resolved Hide resolved
library/SimplePie/Category.php Outdated Show resolved Hide resolved
@Art4 Art4 mentioned this pull request Feb 21, 2022
14 tasks
@Art4
Copy link
Contributor Author

Art4 commented Feb 21, 2022

yeah I'm not sure. Maybe if enough people are keen on this then we could create a version 2.0 starting with this patch? I don't have the time to maintain it at the moment as it seems like there's quite a bit of refactoring to make it worthwhile.

@mblaney I've done the most of the refactoring that would be needed in #722. I've also created a more specific roadmap, how the changes of this PR could released.

@mblaney
Copy link
Member

mblaney commented Feb 22, 2022

Hi @Art4 thanks for the update and thanks @jtojnar for commenting. I agree with what you're saying and don't want to stand in the way of the improvements you're making just because I've been too busy to think about it! :-) I will leave this open for a few more days in case there are any other opinions about such a big change, but otherwise happy to merge.

@Art4
Copy link
Contributor Author

Art4 commented Feb 22, 2022

Thank you @mblaney. I'm fully aware that this changes should be discussed by a wider range of the community. As @jtojnar already stated is this PR specially designed not to introduce a BC break. By releasing this PR as a new minor version (v1.6.0) we will hopefully make more people aware of the upcoming changes from #722 so they could share their opinions.

@mblaney
Copy link
Member

mblaney commented Feb 28, 2022

ok thanks for your work on this @Art4 merging now

@mblaney mblaney merged commit 2f820e1 into simplepie:master Feb 28, 2022
@Art4 Art4 mentioned this pull request Apr 4, 2022
@Art4 Art4 deleted the add-namespace branch April 4, 2022 09:57
@Art4 Art4 mentioned this pull request May 6, 2022
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants