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

[5.1] Update deleted files and folders lists in script.php for 5.1.0-alpha3 #42567

Closed

Conversation

richard67
Copy link
Member

@richard67 richard67 commented Dec 25, 2023

Pull Request for Issue # .

Summary of Changes

This pull request (PR) updates the lists of files and folders to be deleted on update in script.php by the files from PRs #42082 and #42423 for the next alpha release (5.1.0-alpha3).

There are no other deleted files and folders to be added to the lists up to now.

Testing Instructions

Code review.

Or update from 4.4.x or 5.0.x to the latest 5.1 nightly for the actual result and to the patched update package or custom update URL created by Drone for this PR for the expected result.
Patched packages and custom update URL can be found here: https://artifacts.joomla.org/drone/joomla/joomla-cms/5.1-dev/42567/downloads/72766 .

Actual result BEFORE applying this Pull Request

The files and folders added by this PR to the list in script.php are still present after the update.

Expected result AFTER applying this Pull Request

The files and folders added by this PR to the list in script.php have been deleted with the update.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@brianteeman
Copy link
Contributor

is it really b/c to remove those two media files. What if another template is using them?

@richard67
Copy link
Member Author

is it really b/c to remove those two media files. What if another template is using them?

@brianteeman I don't know. @Fedik As it was your PR which removed them from the sources: What do you think? Should we keep them?

If yes, then I have to add an exception to the "build/deleted_file_check.php" script so they are not reported every time as to be deleted.

@richard67
Copy link
Member Author

@brianteeman In previous cases like this we did not keep the js files, see e.g. PR #42364 .

@brianteeman
Copy link
Contributor

thats php not js

@richard67
Copy link
Member Author

richard67 commented Dec 25, 2023

thats php not js

@brianteeman That's right. I asked other maintainers on Mattermost for their opinion. As far as I can see up to now, the JS never was part of any web asset json, so any 3rd party override would have to explicitly load that js file, and it is not very likely that there are template overrides for the multilingual status modal.

@brianteeman
Copy link
Contributor

likeliness is not a consideration in deciding if something follows our b/c policy or not

@richard67
Copy link
Member Author

@brianteeman But then the files shouldn’t have been deleted in the sources with PR #42082 at the first place, or should they?

@brianteeman
Copy link
Contributor

The policy as written https://developer.joomla.org/development-strategy.html says that those files are not covered as they are rendered markup but the js is covered

@richard67
Copy link
Member Author

The policy as written https://developer.joomla.org/development-strategy.html says that those files are not covered as they are rendered markup but the js is covered

@brianteeman But PR #42082 has also removed JS files from media_source.

@richard67 richard67 marked this pull request as draft December 25, 2023 22:09
@richard67
Copy link
Member Author

Changed to draft as long as clarification is ongoing if the files shall be kept or not so the PR doesn't get merged by mistake.

@Fedik
Copy link
Member

Fedik commented Dec 26, 2023

is it really b/c to remove those two media files. What if another template is using them?

This file doing nothing useful, that may need for others. I do not see b/c here.

@richard67
Copy link
Member Author

The policy as written https://developer.joomla.org/development-strategy.html says that those files are not covered as they are rendered markup but the js is covered

@brianteeman You are right, the removed js was not flagged as private: https://github.com/joomla/joomla-cms/blob/481a5d6f9fa65b3adad3d32b57e7d96a6c4e0c0d/build/media_source/mod_multilangstatus/js/admin-multilangstatus.es6.js

Will leave this PR as draft until finally decided or confirmed by the maintainers team and if necessary replace it by a new one to add an exception to the "build/deleted_file_check.php" script and some PR for the deprecations stuff on manual.joomla.org .

@brianteeman
Copy link
Contributor

is it really b/c to remove those two media files. What if another template is using them?

This file doing nothing useful, that may need for others. I do not see b/c here.

usefulness is not a consideration in deciding if something follows our b/c policy or not

@Fedik
Copy link
Member

Fedik commented Dec 26, 2023

Sorry, but this does not make any sense. And I do not see where such case are covered by b/c policy.

If you refering to:

All JavaScript functions and classes that are not flagged as private.

Do you see any function or class in removed file? This file does not containe any of it.
This rule is more about our client side api, about global functions, classes and objects like Joomla, Joomla.Text._() etc.

And btw, I would consider all files under media/plg_x, media/mod_x, and most of media/com_x as private (with some exceptions).
Most of our "public" api is under media/system, rest is private.

@richard67
Copy link
Member Author

I see. I missed the „functions and classes“ part, reading too fast.

@richard67 richard67 changed the title [5.1] Update deleted files and folders lists in script.php for 5.1.0-alpha2 [5.1] Update deleted files and folders lists in script.php for 5.1.0-alpha3 Dec 26, 2023
@richard67 richard67 marked this pull request as ready for review December 26, 2023 18:36
@richard67 richard67 added the RMDQ ReleaseManagerDecisionQueue label Jan 7, 2024
@bembelimen
Copy link
Contributor

Agree with @Fedik here, good to merge.

@richard67
Copy link
Member Author

Agree with @Fedik here, good to merge.

Wait, there are more files coming from PR #42567 . But the discussion is the same for these.

@richard67
Copy link
Member Author

I've updated the PR by the deleted files "media/com_cpanel/js/admin-add_module.js" and "media/com_modules/js/admin-module-edit.js" (plus the minified and gzipped ones).

For them the same applies as already discussed before for the other files and folder: They do not contain any functions or classes so they don't fall under our b/c promise.

See also again @Fedik 's comment above: #42567 (comment) .

@richard67
Copy link
Member Author

Ok, we have just discussed this PR in the CMS Maintainers Team and have decided to play safe. That means this PR here can be closed. Someone (will see if I can find the time) has to provide the necessary documentation for deprecation and removal in 6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-5.1-dev RMDQ ReleaseManagerDecisionQueue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants