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

[6.0] Drop Bootstrap::framework #44991

Open
wants to merge 11 commits into
base: 6.0-dev
Choose a base branch
from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Feb 24, 2025

Pull Request for Issue # .

Summary of Changes

  • B/C break
  • Drop Bootstrap::framework Since J4 any bootstrap (js) component should be specifically loaded, the framework fn was kept for compatibility

Testing Instructions

  • code review
  • Add HTMLHelper::_('bootstrap.framework'); somewhere in the index.php of the atum template
  • Check that all the BS components are registered

Screenshot 2025-02-24 at 11 43 22

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

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: PR

  • No documentation changes for manual.joomla.org needed

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-6.0-dev labels Feb 24, 2025
Copy link
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

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

can you please do only on chance in one pr.

  • you update package-lock.json (version numer)
  • you change code style
  • you use deprecated Factory::getDocument()

and the only thing this PR should do is to remove lines 588 to 616 in the Bootstrap.php

@HLeithner HLeithner added the b/c break This item changes the behavior in an incompatible why. HEADS UP label Feb 24, 2025
@joomla-cms-bot joomla-cms-bot removed the NPM Resource Changed This Pull Request can't be tested by Patchtester label Feb 24, 2025
@laoneo
Copy link
Member

laoneo commented Feb 24, 2025

Istead fo removing, I would move it to the compat plugin.

@Hackwar
Copy link
Member

Hackwar commented Feb 24, 2025

Do we have to add the deprecated tag to the methods in the compat plugin as well?

@dgrammatiko
Copy link
Contributor Author

Do we have to add the deprecated tag to the methods in the compat plugin as well?

To me it feels redundant, as it will never show up in the IDE

@Hackwar
Copy link
Member

Hackwar commented Feb 24, 2025

I agree that it wont show up in the IDE, but it will prevent any discussions in the future about removal and deprecation if you move the tag from the old code also over to the new code.

@Hackwar
Copy link
Member

Hackwar commented Feb 24, 2025

Thank you!

@softforge
Copy link
Contributor

@dgrammatiko I like this change and the addition to the compatibility plugin, to then remove it after.
Can you do as @HLeithner suggested and we can get testing done and merge it

@HLeithner
Copy link
Member

the implementation in the b/c plugin is not optimal.

please add a new Class for this function and also an option to disable "all html helper functions" (do we have more?) and make it as option please.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Feb 24, 2025

please add a new Class

what namespace? is namespace Joomla\Plugin\Behaviour\Compat\HTML\Bootstrap ok?

and also an option

simple toggle should be ok here?

@softforge the first batch of requests is already done, I'll do the lest 2 later on (or tomorrow)

@HLeithner
Copy link
Member

yes namespace is ok, we already have yes/no fields in the config. something like that

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Feb 24, 2025
@dgrammatiko
Copy link
Contributor Author

  • toggler added
  • moved to own class

Screenshot 2025-02-24 at 19 24 54

@Fedik Fedik added the Removal Removes functionality label Feb 27, 2025
@Fedik
Copy link
Member

Fedik commented Feb 27, 2025

I have tested this item ✅ successfully on 30d3736


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44991.

@@ -8,6 +8,8 @@ PLG_COMPAT_FIELD_CLASSES_ALIASES_LABEL="Classes Aliases"
PLG_COMPAT_FIELD_CLASSES_ALIASES_DESCRIPTION="Add class aliases for classes which have been renamed or moved to a namespace."
PLG_COMPAT_FIELD_ES5_ASSETS_DESCRIPTION="Activate this option if your extension requires *.es5 assets which has resulted in an exception. The assets provided are empty but prevent the exception."
PLG_COMPAT_FIELD_ES5_ASSETS_LABEL="ES5 Assets"
PLG_COMPAT_FIELD_HTML_HELPERS_DESCRIPTION="Activate this option if your extension requires deprecated HTMLHelper classes or functions. When enabled, will provide backward compatibility to the prior major version."
Copy link
Contributor

Choose a reason for hiding this comment

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

why not state the version number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried the not original take here and I copied that part from here:

PLG_COMPAT_XML_DESCRIPTION="If you use extensions which are not using the current Joomla Coding standards then this plugin, when enabled, will provide backward compatibility to the prior major version."

@@ -8,6 +8,8 @@ PLG_COMPAT_FIELD_CLASSES_ALIASES_LABEL="Classes Aliases"
PLG_COMPAT_FIELD_CLASSES_ALIASES_DESCRIPTION="Add class aliases for classes which have been renamed or moved to a namespace."
PLG_COMPAT_FIELD_ES5_ASSETS_DESCRIPTION="Activate this option if your extension requires *.es5 assets which has resulted in an exception. The assets provided are empty but prevent the exception."
PLG_COMPAT_FIELD_ES5_ASSETS_LABEL="ES5 Assets"
PLG_COMPAT_FIELD_HTML_HELPERS_DESCRIPTION="Activate this option if your extension requires deprecated HTMLHelper classes or functions. When enabled, will provide backward compatibility to the prior major version."
PLG_COMPAT_FIELD_HTML_HELPERS_LABEL="HTML Helpers"
Copy link
Contributor

Choose a reason for hiding this comment

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

to avoid confusion should this not be specific and refer to boostrap html helpers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, Harald suggested one switch for all (probably there are more, haven't really checked) of them: #44991 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW one or multiple switches is fine by me

@laoneo laoneo removed the Removal Removes functionality label Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
b/c break This item changes the behavior in an incompatible why. HEADS UP Language Change This is for Translators PR-6.0-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants