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

Replace JPATH_PLATFORM with _JEXEC #40343

Merged
merged 7 commits into from
May 29, 2023

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Apr 7, 2023

Replaces JPATH_PLATFORM with _JEXEC as JPATH_PLATFORM. Like that we have one way to check if the CMS is loaded correctly.

@sandewt
Copy link
Contributor

sandewt commented Apr 7, 2023

What to do with this? Consequences? Depreceated?

if (!defined('JPATH_PLATFORM')) {

require_once JPATH_PLATFORM . '/loader.php';

if (!defined('JPATH_PLATFORM')) {

if (!defined('JPATH_PLATFORM')) {

@brianteeman
Copy link
Contributor

Surely this is a breaking change

@laoneo
Copy link
Member Author

laoneo commented Apr 7, 2023

What does break? Happy to solve any bc issues.

@laoneo
Copy link
Member Author

laoneo commented Apr 7, 2023

What to do with this? Consequences? Depreceated?

if (!defined('JPATH_PLATFORM')) {

require_once JPATH_PLATFORM . '/loader.php';

if (!defined('JPATH_PLATFORM')) {

if (!defined('JPATH_PLATFORM')) {

These must stay for backwards compatibility.

@brianteeman
Copy link
Contributor

What does break? Happy to solve any bc issues.

Any tool that people have written to check for the presence of the check will fail.

@laoneo
Copy link
Member Author

laoneo commented Apr 7, 2023

The check should still work as the defines are still in place. @sandewt pointed them out in his last comment. Only the occurrences in the CMS code are changed.

@brianteeman
Copy link
Contributor

The check should still work as the defines are still in place. @sandewt pointed them out in his last comment. Only the occurrences in the CMS code are changed.

it is those occurences I am referring to.

this is a disruptive change for zero benefit and should definitely not be made in a minor release

@laoneo
Copy link
Member Author

laoneo commented Apr 7, 2023

So you agree that it is not a bc break?

@wilsonge
Copy link
Contributor

wilsonge commented Apr 7, 2023

This was proposed in the past and we agreed things in the libraries folder was a b/c break because there were concrete examples of cli apps (in the j3 format) even on our own joomla.org sites defining JPATH_PLATFORM instead of JEXEC.

So yes it's a b/c break. You should be fine with the changes in the administrator folder however.

To be clear I'm a 👍 for merging this for 5.0 however.

@sandewt
Copy link
Contributor

sandewt commented Apr 8, 2023

See, also for completeness:

#39535 (comment)

#40337 (comment)

@laoneo
Copy link
Member Author

laoneo commented Apr 8, 2023

I can't really imagine how you can run joomla 4 without jexec so I wouldn't count this as break. But I do also have no issue to rebase to 5. For me is more important that we get rid of inconsistency.

@brianteeman
Copy link
Contributor

I can't really imagine how you can run joomla 4 without jexec so I wouldn't count this as break.

@wilsonge already said there are cli apps

@laoneo laoneo changed the base branch from 4.4-dev to 5.0-dev April 11, 2023 07:06
@laoneo
Copy link
Member Author

laoneo commented Apr 11, 2023

I did rebase this one #and made a deprecation pr for 4.4 #40366.

@laoneo laoneo mentioned this pull request Apr 11, 2023
4 tasks
@laoneo laoneo removed the PR-4.4-dev label Apr 11, 2023
Copy link
Contributor

@wilsonge wilsonge left a comment

Choose a reason for hiding this comment

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

To be clear this is still technically a b/c break against what production agreed. However I thiink this is a good and positive change and should be merged anyhow.

@laoneo
Copy link
Member Author

laoneo commented Apr 11, 2023

Is there something documented about this vote? I'm not aware of any discussion.

@wilsonge
Copy link
Contributor

I've not been in the meetings since 4.0 finished (so 1.5 years now) but https://volunteers.joomla.org/departments/production/reports/1793-production-dept-meeting-minutes-august-23-2022 was the motion i understood to have been approved which is deprecation for two major versions before removal hence #38657 being merged which was 99% of the originally intended 5.0 deprecations going back to 6.0

@laoneo
Copy link
Member Author

laoneo commented Apr 11, 2023

You was talking before about that PLATFORM should not be deprecated issue. The 2 years policy I'm aware of.

@HLeithner HLeithner added b/c break This item changes the behavior in an incompatible why. HEADS UP Removal Removes functionality labels May 2, 2023
@HLeithner
Copy link
Member

@laoneo can you sync the branch and add a deprecation notice in joomla 4.4 for this /files#diff-af833392184ea677c8d50f3b09187a3602ecb4eaed7b35a622fbf2e96ff1d3e6 (defining the constant) please

@laoneo
Copy link
Member Author

laoneo commented May 29, 2023

Did the sync, but not sure what you want for a notice where?

@HLeithner HLeithner added this to the Joomla! 5.0 milestone May 29, 2023
@HLeithner HLeithner merged commit e2170c8 into joomla:5.0-dev May 29, 2023
@HLeithner
Copy link
Member

Thanks

@laoneo laoneo deleted the plaform/remove branch May 29, 2023 09:50
heelc29 added a commit to heelc29/joomla-cms that referenced this pull request Oct 6, 2024
pe7er pushed a commit that referenced this pull request Oct 20, 2024
* move use statement out of phpcs disable

caused by #43265

* move use statement out of phpcs disable

caused by #40129

* remove blank line in phpcs disable

* fix phpcs enable rule

caused by #40343
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 Removal Removes functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants