#110 [WIP] Add namespaces and, in particular, fix Conflict with Event class and PHP Event module

Open
senooken wants to merge 176 commits from senooken/fix-event-class-conflict into diogo/nightly
senooken commented 4 years ago

I am using GNU social on rental server.

My server has PHP with Event module enabled (extension = event in php.ini or built-in).

But GNU social also has Event class (lib/util.event.php) with no namespace!

So when I install GNU social and access install.php, following error message is shown.

Fatal error: Cannot declare class Event, because the name is already in use in /home/senooken/.local/var/www/html/gnusocial/lib/util/event.php on line 32      

Surely I could fix my php.ini, but my rental server could be disabled Event module due to PHP built-in module.

So I wanted to fix this conflict and I fixed for my rental server.

I fixed this conflict by following steps.

  1. Add namespace GNUsocial; to lib/util/event.php.
  2. Fixing target file are using Event class and no use declaration (ex. use Hoa\Event;).
  3. Replace Event:: to \GNUsocial\Event:: in target files.

I fixed by following shell script.

find . -name "*.php" | while read -r file; do
  [ "$file" = "./lib/util/event.php" ] && continue
  if grep -q 'Event::' "$file" && ! grep -q 'use.*Event.*' "$file"; then
    sed -i 's/Event::/\\GNUsocial\\Event::/g' "$file"
  fi
done

sed -i '/defined/inamespace GNUsocial;\n' lib/util/event.php

Could you check my pull request?

I am using GNU social on rental server. My server has PHP with Event module enabled (`extension = event` in `php.ini` or built-in). But GNU social also has Event class (`lib/util.event.php`) with no namespace! So when I install GNU social and access `install.php`, following error message is shown. ``` Fatal error: Cannot declare class Event, because the name is already in use in /home/senooken/.local/var/www/html/gnusocial/lib/util/event.php on line 32 ``` Surely I could fix my `php.ini`, but my rental server could be disabled Event module due to PHP built-in module. So I wanted to fix this conflict and I fixed for my rental server. I fixed this conflict by following steps. 1. Add `namespace GNUsocial;` to `lib/util/event.php`. 2. Fixing target file are using Event class and no `use` declaration (ex. `use Hoa\Event;`). 3. Replace `Event::` to `\GNUsocial\Event::` in target files. I fixed by following shell script. ``` find . -name "*.php" | while read -r file; do [ "$file" = "./lib/util/event.php" ] && continue if grep -q 'Event::' "$file" && ! grep -q 'use.*Event.*' "$file"; then sed -i 's/Event::/\\GNUsocial\\Event::/g' "$file" fi done sed -i '/defined/inamespace GNUsocial;\n' lib/util/event.php ``` Could you check my pull request?
Diogo Cordeiro commented 4 years ago
Owner

GNU social really should be using namespaces and such addition to our code is wanted, a MR adding namespaces to all of our code would be preferred, which is why I am against merging this for now...

Furthermore, there are some issues with the used shell script, everything under vendor/ is third party code, therefore, changes like the one made to the file vendor/hoa/event/Bucket.php:86 are wrong.

Finally, something to bear in mind is that, being this class directly related to our Event dispatcher, it is often used by third party plugins, therefore, this change will break some of them. On the other hand, v2.0 already breaks some backwards compatibility, there's more to gain from using namespaces. Furthermore, plugins usually are small enough to allow adding namespaces easily by means of use instead of full calls like \GNUsocial\.

Thank you for starting some work on adding namespaces to our codebase and looking forward to contributing :)

GNU social really should be using namespaces and such addition to our code is wanted, **a MR adding namespaces to all of our code would be preferred**, which is why I am against merging this for now... Furthermore, there are some issues with the used shell script, **everything under `vendor/` is third party code**, therefore, changes like the one made to the file `vendor/hoa/event/Bucket.php:86` are wrong. Finally, something to bear in mind is that, being this class directly related to our Event dispatcher, it is often used by third party plugins, therefore, this change will break some of them. On the other hand, v2.0 already breaks some backwards compatibility, there's more to gain from using namespaces. Furthermore, plugins usually are small enough to allow adding namespaces easily by means of `use` instead of full calls like `\GNUsocial\`. Thank you for starting some work on adding namespaces to our codebase and looking forward to contributing :)
senooken commented 4 years ago
Poster

Hi. Thanks for your quick comment. Me too, we should add namespace to all related files at first.

But this is too many change. So in my first step, I will try fixing my problem in this pull request.

I missed vendor/hoa/event/Bucket.php:86 only in vendor/.

In my step 3. Replace Event:: to \GNUsocial\Event:: in target files, I originally inserted use GNUsocial\Event; instead of full prefix.

But I thought full prefix is more smaller modifying line before sent pull request. But for third party plugins, use GNUsocial\Event is better same as your opinion.

If we will introduce namespace for whole code, are there policy?

If all tasks are introducing namespace GNUsocial; in related code, I will try it by creating shell script.

Hi. Thanks for your quick comment. Me too, we should add namespace to all related files at first. But this is too many change. So in my first step, I will try fixing my problem in this pull request. I missed `vendor/hoa/event/Bucket.php:86` only in `vendor/`. In my step 3. Replace Event:: to \GNUsocial\Event:: in target files, I originally inserted `use GNUsocial\Event;` instead of full prefix. But I thought full prefix is more smaller modifying line before sent pull request. But for third party plugins, `use GNUsocial\Event` is better same as your opinion. If we will introduce namespace for whole code, are there policy? If all tasks are introducing `namespace GNUsocial;` in related code, I will try it by creating shell script.
senooken commented 4 years ago
Poster

Firstly, I tried introducing namespace for whole code, but I realized that there are a lot of PHP standard functions. So I gave up in this time.

But I responded to the others comments.

  1. Remove vendor/ for fixing target.
  2. Change full prefix to use GNUsocial\Event;.

I used following shell commands for these fix from nightly branch.

find . -name "*.php" | while read -r file; do
    case "$file" in *vendor/*|*lib/util/event.php) continue; esac
    (! grep -q Event:: "$file" || grep -q 'use GNUsocial\\Event;' "$file") && continue
    sed -i '0,/^[a-zA-Z0-9$]/s/^[a-zA-Z0-9$].*$/use GNUsocial\\Event;\n\n&/' "$file"
done

grep -q 'namespace GNUSocial;' lib/util/event.php ||
    sed -i '0,/^[a-zA-Z0-9$]/s/^[a-zA-Z0-9$].*$/namespace GNUsocial;\n\n&/' lib/util/event.php

I inserted namespace GNUsocial; and use GNUSocial\Event; before 2 lines of first valid code in target file.

I wish I want to introduce whole namespace. But it is challenging work.So I will fix my small problem in first step.

I am watching this nightly branch and my GNU social server (https://social.senooken.jp/senooken) is latest nightly branch since 2 weeks ago (unstable and buggy).

I want to communicate with other ActivityPub server (Mastodon v3 and Misskey). After ActivityPub plugins working well, why don't we try to this namespace problem?

Please check my pull request.

Firstly, I tried introducing namespace for whole code, but I realized that there are a lot of PHP standard functions. So I gave up in this time. But I responded to the others comments. 1. Remove `vendor/` for fixing target. 2. Change full prefix to `use GNUsocial\Event;`. I used following shell commands for these fix from nightly branch. ``` find . -name "*.php" | while read -r file; do case "$file" in *vendor/*|*lib/util/event.php) continue; esac (! grep -q Event:: "$file" || grep -q 'use GNUsocial\\Event;' "$file") && continue sed -i '0,/^[a-zA-Z0-9$]/s/^[a-zA-Z0-9$].*$/use GNUsocial\\Event;\n\n&/' "$file" done grep -q 'namespace GNUSocial;' lib/util/event.php || sed -i '0,/^[a-zA-Z0-9$]/s/^[a-zA-Z0-9$].*$/namespace GNUsocial;\n\n&/' lib/util/event.php ``` I inserted `namespace GNUsocial;` and `use GNUSocial\Event;` before 2 lines of first valid code in target file. I wish I want to introduce whole namespace. But it is challenging work.So I will fix my small problem in first step. I am watching this nightly branch and my GNU social server (https://social.senooken.jp/senooken) is latest nightly branch since 2 weeks ago (unstable and buggy). I want to communicate with other ActivityPub server (Mastodon v3 and Misskey). After ActivityPub plugins working well, why don't we try to this namespace problem? Please check my pull request.
Diogo Cordeiro commented 4 years ago
Owner

I agree that adding namespaces to the current codebase will be a lot of work.

Once proper ActivityPub support is achieved, i.e., we finish our current TODO list, we will go through this during the summer of 2020. That is, we might release v2 before adding namespaces but, v3 will have them.

Thanks you for your contributions! :)

I agree that adding namespaces to the current codebase will be a lot of work. Once proper ActivityPub support is achieved, i.e., we finish our current [TODO list](https://kanban.diogo.site/?controller=BoardViewController&action=readonly&token=03795efb8138c4e7661a900c234c0df1bc3fc03cdfcda8619cd5d0e666de), we will go through [this during the summer of 2020](https://www.diogo.site/projects/GNU-social/soc/2020/). That is, we might release v2 before adding namespaces but, v3 will have them. Thanks you for your contributions! :)
senooken commented 4 years ago
Poster

OK. I will watch TODO list also. I apply in this commit only my server, although it is for lazy for me.... Thanks.

OK. I will watch TODO list also. I apply in this commit only my server, although it is for lazy for me.... Thanks.
This pull request can't be merged automatically because there are conflicts.
Please merge manually in order to resolve the conflicts.
Sign in to join this conversation.
No Milestone
No assignee
2 Participants
Loading...
Cancel
Save
There is no content yet.