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

[4.0] Fix Composer Deprecation Notices #31804

Merged
merged 12 commits into from
Jan 27, 2021
Merged

Conversation

bembelimen
Copy link
Contributor

Pull Request for Issue #29794 .

Summary of Changes

Fixes the composer deprecations using the 3.x code from @SharkyKZ here: #30802

Testing Instructions

Test 1: Update a Joomla! 3.10 installation with the 4.0 installation of this PR (see at the end for download link)
Test 2: Run composer install for this PR
Test 3: Test unit tests

Actual result BEFORE applying this Pull Request

Result 1: (especially on windows systems) the renamed files from this PR have the incorrect case (or old files are still there)
Result 2: composer does throw deprecation notices

Expected result AFTER applying this Pull Request

Result 1: (especially on windows systems) the renamed files from this PR have the correct case
Result 2: composer does not throw any deprecation notices
Result 3: unit test are working

Thanks @SharkyKZ for the 3.0 code
Thanks @HLeithner for helping with this PR

$installer = new JoomlaInstallerScript;

$installer->deleteUnexistingFiles();
$installer->fixFilenameCasing();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite - it's a protected function :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this should be here, there is no reason to have it on 2 places. I thinks It's also wrong in the databasemodel (both deleteUnexistingfiles and fix filename casing).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having it here is the correct place because it runs before the first page load on the new J4 codebase - better than loading the first page as a general rule (as we may always need to add more files to this list in the future for composer things)

@wilsonge wilsonge mentioned this pull request Dec 30, 2020
@astridx
Copy link
Contributor

astridx commented Dec 30, 2020

Is composer version 1 important or can I test it with composer 2?

@bembelimen
Copy link
Contributor Author

bembelimen commented Dec 30, 2020

Is composer version 1 important or can I test it with composer 2?

If you get errors/deprecations there, too, then yes.

@bembelimen bembelimen marked this pull request as ready for review December 30, 2020 17:17
@HLeithner
Copy link
Member

@wilsonge drone is broken now^^

@wilsonge
Copy link
Contributor

This test is in a different class to the one I modified!?

@HLeithner
Copy link
Member

The current error message expains it better:

1) Joomla\Tests\Unit\Libraries\Cms\Image\ImageTest::testGetFilterInstance
RuntimeException: The Filterinspector image filter is not available.

Since you removed the Instance creating it seems the 2 files are related.

Rename file casing
@richard67
Copy link
Member

@bembelimen Could you solve conflicts? Maybe it needs to adapt to the recently merged "dry run" feature.

In addition, it needs to adapt to changes I've made with my PR #32006 , which replaces PR #30802 and contains additional changes on comments and a fix for OSX. If you want I can help with that and make a PR for you after you have done the rest.

@richard67
Copy link
Member

@bembelimen I was checking the conflict shown here for the script.php file and have noticed something.

With PR #31943 , some logging and a dry run option have been added to the files and folder deletion.

This PR here now calls at the end of that function the new one for moving files with wrongly cased file names.

It might make sense to refactor this PR a bit to do the same (logging and dry run option) for the files moving function.

The dry run later could be used e.g. in 4.1 to have something like the database schema check also for the "are there files or folders to be deleted or files to be renamed" checks, with corresponding "Fix" buttons maybe, to de-couple that a bit from database schema fixing.

@richard67
Copy link
Member

@bembelimen I've allowed myself to fix the conflict.

@wilsonge wilsonge merged commit 368a1f3 into joomla:4.0-dev Jan 27, 2021
@wilsonge
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants