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

Reword the Resource access restrictions chapter #165

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cedric-anne
Copy link
Member

No description provided.

Copy link
Contributor

@cconard96 cconard96 left a comment

Choose a reason for hiding this comment

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

English review OK.

Completely disagree with the described functionality.
PHP files shouldn't be in the public directory at all and I see no good reason to support ".js.php" types of assets.

@cedric-anne
Copy link
Member Author

cedric-anne commented Jan 22, 2025

PHP files shouldn't be in the public directory at all

This is only to support legacy scripts. There is a lot of plugins that have PHP files outside their ajax or front directory. They should probably review them to transform them into Symfony controllers, but handling them would permit to give more time to plugin developpers to refactor their code.

I see no good reason to support ".js.php" types of assets.

I agree that having JS files rendered by PHP is not really a good practice, but I think trying to filter resources depending on their type is not our job. As long a a plugin developer puts a resource in the public dir of its plugin, then it clearly indicates that this resource must be public. For instance, the gappessentials plugin has a apirest.php script in its root directory (https://github.com/ticgal/gappessentials/blob/master/apirest.php), and moving it in the public dir of the plugin to make it public permits to not have to change any URL in the applications using this endpoint.

Anyway, I will change the name of the resource to mypluginapi.php. It will be a better example.

@cedric-anne cedric-anne force-pushed the 11.0/public-php-scripts branch from 06859eb to 095a9d2 Compare January 22, 2025 12:43
@trasher
Copy link
Collaborator

trasher commented Jan 22, 2025

Masbe should we just inform users that it's not a god practice? For now, the main problem is we do not have any existing alternative to propose.

@cedric-anne
Copy link
Member Author

Masbe should we just inform users that it's not a god practice? For now, the main problem is we do not have any existing alternative to propose.

We have to write a documentation about how to create a plugin Symfony controller to be able to add a note indicating that legacy scripts should be refactored.
We also have to write a documentation indicating that we have some generic controllers (not yet completely finished) and how to use them.

I guess both should be done in separate PRs, but we can keep the current PR in draft mode for the moment.

@cconard96
Copy link
Contributor

If this is only for scripts that were in the root of a plugin folder and nothing else like the inc and src folders, then I think I misunderstood and it needs clarified in the documentation.

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.

3 participants