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

[ticket/10850] Fixing create_schema_files.php so that it creates popper schema files #775

Closed
wants to merge 1 commit into from

Conversation

brunoais
Copy link
Contributor

Fixed create_schema_files.php by changing the open mode in fopen from "wt" to "wb".

PHPBB3-10850

@naderman
Copy link
Sponsor Member

We use unix line endings everywhere, why would you use \r\n? It should stay \n.

@naderman
Copy link
Sponsor Member

Oh ok, nevermind I hadn't read the ticket. Removing t and hardcoding which line endings we want makes a lot of sense.

So only question is, do we want it to be \r\n?

@bantu
Copy link
Collaborator

bantu commented Apr 26, 2012

Replace t with b to open the file in binary mode? See http://php.net/manual/en/function.fopen.php For portability, it is strongly recommended that you always use the 'b' flag when opening files with fopen().

@bantu
Copy link
Collaborator

bantu commented Apr 26, 2012

Line endings should be \n unless otherwise required. So the question is whether those DBMSes require anything other than \n.

@brunoais
Copy link
Contributor Author

@naderman
I don't know. But the current files (in dev branch) in the install/schemas use \r\n as their line ending.

@bantu
1.
If you want to be able to read those files using notepad++, gedit and such, that's a bad idea.
Also, if you change to using the "b" modifier, then you'll need to use the "b" modifier while reading the file while installing.

I don't know if they require line endings using \r\n.

@bantu
Copy link
Collaborator

bantu commented Apr 26, 2012

[15:06:59] The php files are all \n
[15:07:29] notepad++ etc. handles \n just fine. we don't care about windows notepad.
[15:07:43] so schema files should be \n too
[15:07:51] (unless otherwise required by the DBMSes)
[15:08:39] installer probably does not use fopen() to read the file, but file_get_contents()
[15:08:51] it's a text file after all and doesn't make a difference
[15:08:57] (unless otherwise required by the DBMSes)

@bantu
Copy link
Collaborator

bantu commented Apr 26, 2012

To me it looks like everything that needs to be done here is replace the t with a b. The other changes are not required. Also, the schema files are actually in \n in the repository.

@brunoais
Copy link
Contributor Author

the problem is worse than I thought.
By default git replaces the "\n" with "\r\n" in windows. So when it's time to solve this for the heredoc with newlines, everything goes haywire!
In the version in Linux users, it shows as "\n" in windows users, it shows "\r\n"... What a mess -_-'

@naderman
Copy link
Sponsor Member

That depends on your git configuration. For phpBB you should really configure it to use newlines only. Preferably also on windows.

Now the real question that needs to be solved here is, as Andreas said, do we even need any \r\n in those schema files, or can we just leave them as \n everywhere anyway, just like we do for all other files?

@p
Copy link
Contributor

p commented Apr 26, 2012

Everything other than mssql should work fine with unix line endings I'm assuming. Mssql should be tested.

@Noxwizard
Copy link
Member

The files are never fed directly to the database, so the line endings aren't going to cause problems. Processing is done on the file first (\r's are removed), then the individual queries are pulled out into an array based on their delimiter, and then they are each executed by the dbal.

@p
Copy link
Contributor

p commented Apr 26, 2012

If so we should have linefeeds only in all files.

@brunoais
Copy link
Contributor Author

@brunoais
Copy link
Contributor Author

brunoais commented May 1, 2012

I've made some tests here and I found out why I was getting gibberish when I used the "wb" mode in fopen. It had to do with an option I tweaked (I don't remember why) in php. Changing to "wb" solved the problem but maybe not by the reasons most people think.
As it is now, in the windows platform, schema files file the oracle_schema.sql have a mix of "\n" and "\r\n" as their newline marker (you can check it). This is not something that breaks files because we are relying on the git's auto "\r\n" to "\n" converter which converts all the "\r\n" into "\n" and leaves the current "\n" alone. So when oracle_schema.sql is uploaded, it is uploaded only with \n as its newline marker. I think this is something important to point out and register for future reference so that we can investigate if this situation has been changed if this file breaks again (cease to function as expected).

@naderman
Copy link
Sponsor Member

naderman commented May 1, 2012

We do not rely on git doing that. You should set up your git correctly, so it checks out all line endings as \n. And you should not be using any \r\n, also on windows.

@brunoais
Copy link
Contributor Author

brunoais commented May 1, 2012

@naderman
Such configuration is not stated in the wiki. Could we use an .attributes file?

@naderman
Copy link
Sponsor Member

naderman commented May 1, 2012

It is stated in the wiki: http://wiki.phpbb.com/Git#Windows

None of us saw a point in forcing this on anyone, hence no gitattributes. But if you don't want \r\n, then configure your git not to give you \r\n.

@brunoais
Copy link
Contributor Author

brunoais commented May 1, 2012

It is stated in the wiki: http://wiki.phpbb.com/Git#Windows

"Quick info for those with TortoiseGit (and used to working with TortoiseSVN) "
Doesn't sound like it. It is clearly stated that it's for the ones that use TortoiseSVN. I don't use TortoiseSVN, so I ignore it and it's normal to ignore such instruction. An error while writtin gin the wiki perhaps...?

@naderman
Copy link
Sponsor Member

naderman commented May 1, 2012

Well it's a wiki. Go change it to state that you should always change autocrlf on windows, even if you don't use tortoise.

@brunoais
Copy link
Contributor Author

brunoais commented May 1, 2012

This PR should be complete. Please check if everything is working as expected.

@naderman done

@p
Copy link
Contributor

p commented May 1, 2012

The two commits have different ticket numbers. They should be squashed into one commit with the appropriate ticket number selected.

The change from text to binary seems sensible given that we expect all files to have unix line endings on windows.

This pr should obviously be tested on windows.

@brunoais
Copy link
Contributor Author

brunoais commented May 1, 2012

  1. Fixed
  2. Well... In order to have that. You must follow: http://wiki.phpbb.com/Git#Windows so that git does not translate "\n" to "\r\n" while downloading from github. Else, you get some of the .sql schema files with "\r\n" in some places which git then translates to "\n" while uploading to the server.
  3. I tested on my windows platform and it worked. I tried with autocrlf set to on and with it set to input. No problems at all were spotted.

@p
Copy link
Contributor

p commented May 2, 2012

Please squash the commits into one.

Changed the fopen mode from "wt" to "wb" as requested in the PR.
The objective behind this is to prevent writting stuff like "\r\r\n" in
windows

PHPBB3-10850
@brunoais
Copy link
Contributor Author

brunoais commented May 2, 2012

@p Does that look ok?

@bantu
Copy link
Collaborator

bantu commented Jun 28, 2012

Merged as 7fba5a0

@bantu bantu closed this Jun 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants